Move main graph to new API v2 endpoint#6152
Open
RobertJoonas wants to merge 13 commits intomasterfrom
Open
Conversation
This is currently only used by the main graph, which is going to move to a new endpoint in this PR. * The CSV export is currently ignoring comparisons * the `&compare=previous_period` option in Stats API v1 is ignored by the timeseries endpoint
* For time:hour and time:minute, sessions are smeared using time_slots. The fix is to filter out time_slots that fall outside of the utc boundaries * For any other time dimension, there's no session smearing, but since sessions are put into time buckets by the last event timestamps, the query might return buckets that are outside of the query time range. The fix is to clamp those sessions into the last bucket instead.
b75763c to
e643b00
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Needs a rebase and most likely in conflict with frontend changes. Builds on top of an intermediate state of #6119.
New comparison response format for timeseries data
Returning comparison data in time buckets was not implemented before. This PR implements it. Unlike comparisons with empty dimensions or non-time dimensions, the number of dimension groups returned in
resultsis guaranteed to match between original and comparison results.["event:page"]), an IN filter guarantees that comparison results is a subset, and can never include a dimension group that's not in the original results.Therefore, in these two cases, it's very easy to attribute a comparison object to each item (row) in the results list.
With a time dimension however, there might be a situation where comparison results contain less/more rows than the original results. For that reason, when constructing the final results list, we iterate over time labels instead of main results. And not just the main query time labels. If the
comparison_time_labelshave greater length, then they also make the results list grow, filling the original results withnilrows. (Seequery_test.exsin diff to get a better idea of how it works).It's also important to note that ClickHouse queries are not returning groups without any data. Therefore, in order for the response to be able to indicate which comparison bucket corresponds to which original bucket, those groups need to be filled with empty metrics. This was also working like that before.
Whenever comparisons are not queried though, the results list doesn't include those empty metric buckets. Therefore, the frontend code (which is not yet implemented) needs to account for that (cc @apata):
plotandcomparison_plot. There'smeta.time_labelsin the response, but nometa.comparison_time_labels. comparison time labels should be read from the comparison object in each row (thedimensionskey).results.find(r => r.dimensions[0] == time_label)returns a row, read it's metrics, otherwise, it's up to the frontend to fill that gap in the plot with an "empty metric" value. Thinking of it now, it sounds easier to solve this problem on the backend (wouldn't have to duplicate the logic of "what is an empty value for this metric" on the frontend either). If we decide to go that way though, we'll want to keep this exclusive to the internal API.Tests
Changelog
Documentation
Dark mode