-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
fix(GitHub Integration): Correctly extract the start/end timestamps from time picker #15
Conversation
…ame graph container For reasons I don't understand, doing this trick to switch the flame graph container mode results in the component continuously rerendering. Aside from obvious performance problems, this component with also trigger network calls when rerendering, resulting in its contents becoming basically useless.
…lues from the url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/pages/ProfilesExplorerView/exploration-types/SceneServiceFlameGraph/SceneFlameGraph.tsx
Outdated
Show resolved
Hide resolved
src/shared/components/FunctionDetailsPanel/infrastructure/useFetchFunctionsDetails.ts
Outdated
Show resolved
Hide resolved
…lues from the url
…iles into gh-explore-profiles
timelineAndProfileApiClient.setLastTimeRange(globalTimeRangeState.state.value); | ||
} | ||
|
||
const timeRangeSubscription = globalTimeRangeState?.subscribeToState((newState) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this will not work as expected. E.g. switching from cpu to goroutine will not update the timerange.
The correct solution is to listen to data loading changes:
const data = buildTimeSeriesQueryRunner({});
...
data.subscribeToState((newState) => {
if (newState.data.state === LoadingState.Loading) {
/// capture time range
}
});
✨ Description
Since #5, we stopped collecting the
start
andend
values from the url state correctly. This meant we passed emptystart
andend
values to the backend when callingSelectMergeProfile
to get a profile for a time range and stack trace. In turn, the backend would pass back an empty response because the time range (0 to 0) didn't match any blocks.📖 Summary of the changes
This PR fixes how the UI sets the
start
andend
when makingSelectMergeProfile
requests. It also fixes a rendering but where the flame graph scene would continuously re-render. It also correctly setskeepCookies
in the sample provisioned data sources.🧪 How to test?
Run locally