Fix Trace List with variable #132
Conversation
iwysiu
requested review from
sunker and
fridgepoet
and removed request for
a team
|
|
|
Backend code coverage report for PR #132 |
|
Frontend code coverage report for PR #132
|
yarn.lock
Outdated
| @@ -3841,17 +3841,10 @@ caniuse-api@^3.0.0: | |||
| lodash.memoize "^4.1.2" | |||
| lodash.uniq "^4.5.0" | |||
|
|
|||
| caniuse-lite@^1.0.0, caniuse-lite@^1.0.30000981, caniuse-lite@^1.0.30001109, caniuse-lite@^1.0.30001135: | |||
There was a problem hiding this comment.
was this change intentional? I don't see a change to package.json so it's a bit strange to see a yarn.lock change right?
There was a problem hiding this comment.
This was because drone was complaining about it and I was messing with it https://drone.grafana.net/grafana/x-ray-datasource/86/1/5
I'll leave it for another commit
| }); | ||
| it('sets the query type to getTrace if query is variable for a traceID', () => { | ||
| const queryType = queryTypeOptionToQueryType([queryTypeOptions[0].value], '$variable', { | ||
| variable: { text: '1-5f048fc1-4f1c9b022d6233dacd96fb84', value: '1-5f048fc1-4f1c9b022d6233dacd96fb84' }, |
There was a problem hiding this comment.
wouldn't the variable be something more like "traceId" rather than an actual traceId?
There was a problem hiding this comment.
The form uses a regex to check for something in this format to determine what query type to use, so the value does have to be shaped like an actual traceId. (the string being passed in as the query is $variable)
| import { DataSourceInstanceSettings, ScopedVars, VariableModel } from '@grafana/data'; | ||
| import { TemplateSrv } from '@grafana/runtime'; | ||
|
|
||
| jest.mock('@grafana/runtime', () => { |
There was a problem hiding this comment.
it seems like you're updating a mock but not any tests in this file. Is that intentional?
There was a problem hiding this comment.
Yeah, this also needed GetTemplateSrv() to be mocked.
| import { TemplateSrv } from '@grafana/runtime'; | ||
| import { ScopedVars, VariableModel } from '@grafana/data'; | ||
|
|
||
| jest.mock('@grafana/runtime', () => { |
There was a problem hiding this comment.
is there a way to just mockout getTemplateSrv ? It seems like that's the only bit you're using right?
There was a problem hiding this comment.
switched it to a jest.SpyOn
The query editor uses a check for
trace idto determine if aTrace Listquery is agetTraceorgetTraceSummariesquery. Previously, it did not replace variables before checking, which meant that a variable that represented a trace id would incorrectly get sorted as agetTraceSummariesquery.Structure-wise it would probably be better if the query function determined which type it was, but I wasn't sure we wanted to do that much restructuring.
Fixes #131