ref(explore): Improve conversion to TimeSeries
#100532
Conversation
- correctly parse `groupBy`, `isOther`, and `yAxis`
github-actions
bot
added
the
Scope: Frontend
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
…ter-incorrectly-treats-cache-keys-as
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| const groupKeys = fields; | ||
| const groupValues = groupName.split(','); |
There was a problem hiding this comment.
This makes the assumption that each group value won't have any commas.
There was a problem hiding this comment.
🤔 it does make that assumption, yes. I don't think there's actually any way for me to figure out which commas are the delimiters, even knowing how many fields there were originally ... but this problem will go away when we use /events-timeseries/ which separates each field completely
There was a problem hiding this comment.
I ended up making this more permissive, and creating empty keys/values when necessary. The values end up getting concatenated with commas later anyway and the keys are not readily visible to anyone, so we should be good. Good catch, thanks!
| if (groupName === 'Other') { | ||
| timeSeries.meta.isOther = true; | ||
| } |
There was a problem hiding this comment.
Feels like the backend should mark some meta to indicate this instead of continuing to relying on the fact that the series name is Other but for now, this is fine.
There was a problem hiding this comment.
That is exactly the case for the /events-timeseries/ endpoint! Once we switch over, we won't have to mark it manually 👍🏻
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
gggritso
deleted the
georgegritsouk/dain-119-legend-name-formatter-incorrectly-treats-cache-keys-as
branch
Explore makes requests to
/events-stats/but immediately converts that response to an array ofTimeSeriesobjects, which are passed to charts and so on. The way it's done right now is a slight hack, which I'm fixing. If the request is a "top events" grouped by environment, we end up withTimeSeriesobjects that look like this:This works fine because
TimeSeriesWidgetVisualizationparses theyAxisvalue and formats it correctly. A better approach is to actually parse the response properly, and create a fullTimeSeriesobject that looks like this:If we correctly populate
yAxis,groupBy, andisOther, that allowsTimeSeriesVisualizationto correctly label everything by default. This removes the need for setting aliases, and improves compatibility. Incidentally, it also fixes DAIN-119! With a propergroupBy, we don't need to try and parse everything as a version, and cause false positives.Other than the fix for that small bug, this doesn't create any visible changes in the UI. This also helps me prepare for replacing
/events-stats/with/events-timeseries/on this page.NOTE: This change creates a bit of duplication when parsing
groupBy, but don't worry, it's temporary, since all this code will go away once we use/events-timeseries/here.Note
Refactors timeseries transformation to populate yAxis/groupBy/isOther and ordering, updates chart/formatter to use meta.isOther, adds parseGroupBy, and removes legacy isTimeSeriesOther.
transformToSeriesMapandconvertEventsStatsToTimeSeriesDatato setyAxis,groupBy(viaparseGroupBy),meta.isOther, andmeta.order; support grouped multi-series; return series grouped byyAxisand sorted byorder.formatTimeSeriesNamereturns"Other"whenmeta.isOtherand continues existing measurement/version/equation formatting.chartVisualizationcolors series usings.meta.isOtherand removes alias-based legend formatting.TimeSeriesGroupByfromwidgets/common/types.parseGroupBywith tests; removeisTimeSeriesOtherand its tests; add tests forOtherhandling in formatter.Written by Cursor Bugbot for commit ea8e062. This will update automatically on new commits. Configure here.