refactor: use new group limit in explorer #698
Conversation
| groupByLimit: this.request.groupByLimit | ||
| }; | ||
| protected buildRequestState(): ExploreRequestState | undefined { | ||
| // Unused since fetchResults is overriden, but abstract so requires a def |
There was a problem hiding this comment.
This one's a bit weird. This is an abstract method used in the parent's fetchResults which we override in this class. It's not really relevant to this implementation since fetchResults uses an observable of state rather than building its own.
This was actually breaking the explorer, (although no one noticed) since the previous implementation was mapping the response data based on the initial state, not the current one.
There was a problem hiding this comment.
Yeah. AFAIR, the ExploreVisualizationRequest did not have all the latest request data. I think we emit on exploreQuery$: Observable<TimeUnaware<GraphQlExploreRequest>>; with latest interval data (possibly more). When i tested it last, using ExploreVisualizationRequest directly did break few use cases for me. So for correctness, I kept it the same way for this data source (but fixed the dependency in the parent data source).
There was a problem hiding this comment.
If this is working, then you don't need to override fetchResults. Just return this.request inside buildRequestState
There was a problem hiding this comment.
As a user interacts with the explorer, all of the fields on ExploreVisualizationRequest may change, and the new queries are emitted on that observable - so we should never really be reading from ExploreVisualizationRequest directly (it's basically like the angular router snapshot vs observable state, but with more confusing naming), but we also can't build state from it either - we should always use the latest from the observable. One way to see the current issue is going to the explorer, selecting a group by key and removing the interval. Instead of getting a segmented aggregation, nothing renders (because we process the result thinking it still has an interval).
There was a problem hiding this comment.
ah okay. It will still be confusing to a new developer. Do you think we should change the design a little bit (as a tech debt)?
There was a problem hiding this comment.
yeah, for sure.
Codecov Report
@@ Coverage Diff @@
## main #698 +/- ##
==========================================
- Coverage 85.17% 85.16% -0.02%
==========================================
Files 782 782
Lines 16056 16042 -14
Branches 2056 2048 -8
==========================================
- Hits 13676 13662 -14
Misses 2347 2347
Partials 33 33
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| selections: requestState.series.map(series => series.specification), | ||
| context: requestState.context, | ||
| limit: requestState.groupByLimit ?? 100, | ||
| limit: requestState.resultLimit, |
There was a problem hiding this comment.
On the consumer side, do we need to change the existing limit in modelJsons (may be) since this will now limit the number of results?
There was a problem hiding this comment.
None exist in the UI as of today (besides what you've got in flight), but in general yes - new data models that extend this will have to use the two types of limit appropriately.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
Uses the new group limit parameter in explorer api instead of the deprecated row limit behavior.
Testing
Visual verification, updated tests
Checklist: