Refactor ad hoc variable processing #399
Conversation
idastambuk requested review from iwysiu and njvrzm and removed request for a team 
| expect(mockedSuperQuery).toHaveBeenCalled(); | ||
| }); | ||
| | ||
| it('should send interpolated query to backend', () => { |
There was a problem hiding this comment.
moved these tests below, to test applyTemplateVariables
| } | ||
| | ||
| // Frontend flow | ||
| const targetsWithInterpolatedVariables = request.targets.map((target) => { |
There was a problem hiding this comment.
Just added interpolating template variables because adhoc filters are later added for Lucene and PPL in the frontend flow, e.g.
this.queryBuilder.buildPPLQuery(target, adhocFilters, queryString)
iwysiu left a comment
iwysiu left a comment There was a problem hiding this comment.
When I hit + twice for a value in the dashboard panel it didn't remove the filter, but in Explore it did. Is that an expected difference (because of the migration) or is that a bug?
| return lucene.toString(removeNodeFromTree(ast, node)); | ||
| } | ||
| | ||
| function removeNodeFromTree(ast: AST, node: NodeTerm): AST { |
There was a problem hiding this comment.
It might make sense to group all the tree functions together and all the query functions together
| value: filter.options.value, | ||
| operator: '!=', | ||
| }; | ||
| // If the opposite filter is present, remove it before adding the new one. |
There was a problem hiding this comment.
It seems like the logic is
FILTER_FOR: remove if present, add if not
FILTER_OUT: remove opposite if present and add filter (always)
I'm not familar with this feature, is that how it's supposed to be? Is FILTER_OUT the only time when we care if the opposite exists, and it doesn't care if it's already been added?
There was a problem hiding this comment.
As far as I understand, that's the case for ad hoc filters, yes. Filter out is basically "only show != value"
This is expected, should have mentioned it. Grafana in those cases still passes the same filters without removing existing ones. I assume this is because you can remove the filters from the variable list above the panels. |
iwysiu left a comment
iwysiu left a comment There was a problem hiding this comment.
Just a couple more new lines
| "resolvedVariable | where `bar` = 'baz' and `job` != 'grafana' and `bytes` > 50 and `count` < 100 and `timestamp` = timestamp('2020-11-22 16:40:43.000000')" | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
nit: newlines before and after this describe block
| expect(query).toBe("`test` = 'test1'"); | ||
| }); | ||
| }); | ||
| describe('Escaping characters in adhoc filter', () => { |
There was a problem hiding this comment.
nit: newline before and after this describe block
3 participants
What this PR does / why we need it:
Previously, we were interpolating variables in the
query()method manually. This adds several methods to the datasource that are called by code Grafana whenever variable and ad hoc interpolation is necessary:applyTemplateVariables- this is a method that is called by Grafana during the super.query() call, which means we don't have to do this manually in our datasource. The method implements template variable interpolation as well as ad hoc queries following the approach in Elastic search from this PRinterpolateVariablesInQueriescalled from multiple places in core grafana, didn't look too much into it but most datasources implement it. Elastic hereWhich issue(s) this PR fixes:
Fixes #379, #389
Special notes for your reviewer:
How to test
+adds a filter+again removes a filter-filters out a field.query.query. The query won't have filters in the UI, just the query inspector