fix: public urls with computed measures and dependant measures are hidden #6520
Conversation
|
FYI it's |
|
I used |
AdityaHegde
changed the title
AdityaHegde
force-pushed
the
fix/advanced-measures-public-url
branch
from
d72c4ce to
e6a6249
Compare
begelundmuller
requested review from
ericpgreen2
and removed request for
begelundmuller
|
@ericpgreen2 Do you want to do this review? I don't have much context around this code. |
| @@ -145,10 +147,11 @@ export const getFilteredMeasuresAndDimensions = ({ | |||
| }); | |||
| if (skipMeasure) return; | |||
| measures.add(measureName); | |||
| measure.referencedMeasures?.filter((refMes) => measures.add(refMes)); | |||
| if (!measure.window) nonWindowMeasures.add(measureName); | |||
There was a problem hiding this comment.
I'm having trouble following this function.
- What is a "filtered" measure and dimension?
- I'm nervous about this line. Adding measures to the list of user-selected measures feels like exactly what caused the original bug. Double-checking... are you sure this is necessary?
There was a problem hiding this comment.
Adding these measures break the totals query, since we dont really get totals data anyway (no data can be seen) it is better to filter them out.
Would be great to add E2E once the setup PR is merged.
There was a problem hiding this comment.
It seems one way this function is overloaded is that it's preparing two different query requests: totals and timeseries, which each have a different set of measures & dimensions.
Can we split this into two functions:
getMeasuresAndDimensionsForTimeSeriesRequest()getMeasuresForTotalsRequest()
There was a problem hiding this comment.
Updated the method, added a param to include/exclude window measures. I dont think the method should differentiate between timeseries vs totals request.
| const measures = new Set<string>(); | ||
| measureNames.forEach((measureName) => { | ||
| const measure = metricsViewSpec.measures?.find( | ||
| (m) => m.name === measureName, | ||
| ); | ||
| // temporary check for window measures until the PR to move to AggregationApi is merged | ||
| if (!measure || measure.requiredDimensions?.length || !!measure.window) | ||
| if ( | ||
| !measure || | ||
| measure.type === | ||
| MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON || | ||
| (!includeWindowMeasures && measure.window) | ||
| // TODO: we need to send a single query for this support | ||
| // (measure.type === | ||
| // MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON && | ||
| // (!dashboard.showTimeComparison || | ||
| // !dashboard.selectedComparisonTimeRange)) | ||
| ) | ||
| return; | ||
|
|
||
| let skipMeasure = false; | ||
| measure.requiredDimensions?.forEach((reqDim) => { | ||
| if ( | ||
| reqDim.timeGrain !== V1TimeGrain.TIME_GRAIN_UNSPECIFIED && | ||
| reqDim.timeGrain !== exploreState.selectedTimeRange?.interval | ||
| ) { | ||
| // filter out measures with dependant dimensions not matching the selected grain | ||
| skipMeasure = true; | ||
| return; | ||
| } | ||
| }); | ||
| if (skipMeasure) return; | ||
|
|
||
| measures.add(measureName); | ||
| measure.referencedMeasures?.filter((refMes) => measures.add(refMes)); | ||
| }); |
There was a problem hiding this comment.
Looks like this should be a measures.filter() statement
There was a problem hiding this comment.
We dont need referencedMeasures anymore. That was the big fix in the PR. The earlier code should have been a forEach.
There was a problem hiding this comment.
Sorry, GitHub's UI made this comment confusIng... I meant the whole code block that's highlighted. At the top-level, this code is initializing an empty set const measures = new Set(), then building up an array. Rather, I'd expect the input measureNames to be filtered like measureNames.filter(...).
There was a problem hiding this comment.
Ah it was adding referencedMeasures before so it was not a filter. Will change in a follow up PR.
…dden (#6520) * Add requiredMeasures to explore fields * Do not send referenced measures for non-window measures * Fix windowed measures * Fix lint+tests * Refactor filter method * Change function name
If a public url is created by selecting a computed measure but not the measure it is computed on, the dashboard fails to load since all required measures are not added to the token.
So we need to add allreferencedMeasuresto fields as well. This has a side effect of showing measures that were not selected.We do not need to send the
referencedMeasuresfor non-window computed measures.