-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Adding comparison to throughput chart #90128
[APM] Adding comparison to throughput chart #90128
Conversation
577d70a
to
77ff058
Compare
9376c67
to
78bee41
Compare
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine merge upstream |
x-pack/plugins/apm/public/components/shared/time_comparison/use_time_range_comparison.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/shared/time_comparison/use_time_range_comparison.ts
Outdated
Show resolved
Hide resolved
@@ -53,19 +52,19 @@ interface SetupRequestParams { | |||
/** | |||
* Timestamp in ms since epoch | |||
*/ | |||
start?: string; | |||
start?: number; |
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.
Why was this changed? It's not a biggie but does make the url a bit harder to read.
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.
Nothing changes in the URL @sqren, I created a new runtime to convert from string (ISO) to number (timestamp). That's why this is now a number.
…emarcondes/kibana into apm-time-comparison-throughput-chart
).toEqual([]); | ||
}); | ||
|
||
it('returns empty array when current period timeseries is empty', () => { |
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.
Thanks for adding this! IMHO this is a bug. At least, I'm not sure why this function would not return data for the previous period if there is data. Instead of merging buckets, have you considered offsetting the data for the previous period by the time difference between end
and comparisonEnd
(or start
and comparisonStart
, not sure what the implications are)?
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.
@dgieselaar I changed the merge function to calculate the x-axis from the previous timeseries based on the time difference (currentPeriodStart and previousPeriodStart)
@elasticmachine merge upstream |
…arison-throughput-chart
…emarcondes/kibana into apm-time-comparison-throughput-chart
I've fixed the issue with the runtime type, converted comparisonType to use an enum and change the labels from yesterday/a week ago to day before/week before. Additionally I fixed a bug where the throughput value for the charts was not correct because it was divided by the entire time range instead of the bucket size (not related to this PR). I'll merge this when CI is green. |
…arison-throughput-chart
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Backport result
|
This reverts commit 874fadf.
This reverts commit 874fadf.
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Cauê Marcondes <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
closes #90570
time.comparison.mov