Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Scenes performance: Measurement #858

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Scenes performance: Measurement #858

wants to merge 18 commits into from

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Aug 2, 2024 •
edited by oscarkilhed
Loading

Ref https://github.com/grafana/hyperion-planning/issues/69

Read about the details in https://docs.google.com/document/d/1AXnxWJju0xS4zZWZUlFPONPKbMpEUYDHqvx0av3SyvE/edit?usp=sharing

  • Figure out circular dep introduced here
  • Handle slow network scenarios, where the measurement can complete sooner than expected.
  • Figure out how do we test SceneRenderProfiler.
📦 Published PR as canary version: 5.25.1--canary.858.11930656504.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]
yarn add @grafana/[email protected]

@dprokop dprokop added patch Increment the patch version when merged release Create a release when this pr is merged labels Aug 2, 2024
@dprokop dprokop mentioned this pull request Aug 2, 2024
Open
5 tasks
@dprokop dprokop changed the title WIP Scenes perf: Measurement Scenes performance: Measurement Sep 13, 2024
@dprokop dprokop marked this pull request as ready for review September 13, 2024 12:43
dprokop
dprokop commented Sep 26, 2024
View reviewed changes
packages/scenes/src/behaviors/SceneRenderProfiler.ts Outdated
}

// Will calculate total time spent on Network
function calculateNetworkTime(requests: PerformanceResourceTiming[]): number {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Add tests

dprokop
dprokop commented Sep 26, 2024
View reviewed changes
packages/scenes/src/querying/registerQueryWithController.ts Outdated

// Wraps an arbitrary Promise in an observble that emits Promise state
export function wrapPromiseInStateObservable(promise: Promise<any>): Observable<QueryResultWithState> {
console.log('wrapPromiseInStateObservable', promise);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log('wrapPromiseInStateObservable', promise);

dprokop
dprokop commented Sep 26, 2024
View reviewed changes
packages/scenes/src/querying/registerQueryWithController.ts
@@ -46,3 +46,30 @@ export function registerQueryWithController<T extends QueryResultWithState>(entr
});
};
}

// Wraps an arbitrary Promise in an observble that emits Promise state
export function wrapPromiseInStateObservable(promise: Promise<any>): Observable<QueryResultWithState> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Add test

@oscarkilhed
Copy link
Contributor

There's a lot of changes here that's just code formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscarkilhed oscarkilhed oscarkilhed approved these changes

@torkelo torkelo Awaiting requested review from torkelo

@ivanortegaalba ivanortegaalba Awaiting requested review from ivanortegaalba

Assignees
No one assigned
Labels
patch Increment the patch version when merged release Create a release when this pr is merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dprokop @oscarkilhed