-
Notifications
You must be signed in to change notification settings - Fork 12.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
datatrails: persist search terms and use them to reduce retrieved metric names #87884
Conversation
50f08ea
to
0a7345c
Compare
e45aa29
to
5915eb8
Compare
export interface MetricsSearchTermsVariableState extends SceneVariableState { | ||
terms: string[]; | ||
} | ||
|
||
export class MetricSearchTermsVariable | ||
extends SceneObjectBase<MetricsSearchTermsVariableState> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we needs to be a new type of variable, why not just use a CustomVariable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can get everything I really need via CustomVariable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I need is a variable that stores a list of search terms that has been typed in, but interpolates these differently:
- When persisting the search terms in the URL, I just want to interpolate this as a simple comma-separated list (for URL readability -- not a hard requirement, any simple format would do)
- When requesting a list of metric names that match these search terms, I want to this variable to be interpolated into a regular expression that the request to Prometheus will make use of
I was struggling to find a way to override the formatting as I do in getValue()
. It seems I can provide value
with a formatter, but it seems I would have to provide a formatter function each time I set state value
.
Looking more closely at CustomVariable, it just looks like a list of preset custom options you could select one of more of. If that's the case, it might not be what I'm looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I can extend the TextBoxVariable instead. Maybe I can even render it on the MetricSelectScene directly, simplifying code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@torkelo
I have simplified much of the code here by extending TextBoxVariable
in 2ecb62a.
eb16622
to
2ecb62a
Compare
); | ||
} | ||
|
||
export function createJSRegExpFromSearchTerms(searchTerms: string[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what don't get is why this searchTerms is not simply the plain searchQuery string, you can do searchQuery.split(' ' ) split here and build the regex from the terms, same inside createPromRegExp.
Then no need for the custom variable, custom URL sync, etc.
(technically no need for variable, just a state prop that is URL synced). But a variable could be easier as you get the URL sync for free, not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what don't get is why this searchTerms is not simply the plain searchQuery string
No longer any reason. I am going to change it to be so.
then no need for the custom variable
Hmm, yes... Here is how it's used currently.
const match = sceneGraph.interpolate(trail, '{${filters}${metricSearch:filter}}');
Arguably, I could construct this query parameter without metricSearch
being an interpolated variable.
custom URL sync, etc.
Yeah, that's already gone. Much less to worry about as full string.
But a variable could be easier as you get the URL sync for free
We could have searchQuery
be a state prop on DataTrail, to ensure that it persists when metric select scene gets wiped. It shouldn't be hard to urlSync this alongside metric
in DataTrail. Hoping it's just a matter of appending the key to the list.
export class MetricSearchTermsVariable extends TextBoxVariable { | ||
constructor(initialState: Partial<{ value: string }>) { | ||
super({ | ||
value: '', | ||
hide: VariableHide.hideVariable, | ||
name: VAR_METRIC_SEARCH_TERMS, | ||
...initialState, | ||
}); | ||
} | ||
|
||
getValue(): VariableValue { | ||
const filtersVar = sceneGraph.lookupVariable(VAR_FILTERS, this); | ||
|
||
const { value } = this.state; | ||
let separator = ','; | ||
|
||
if (filtersVar instanceof AdHocFiltersVariable) { | ||
const value = filtersVar.getValue()?.valueOf(); | ||
if (!value) { | ||
separator = ''; | ||
} | ||
} | ||
|
||
return { | ||
formatter: (format: Format) => { | ||
switch (format) { | ||
case 'filter': { | ||
const terms = deriveSearchTermsFromInput(value); | ||
const regex = createPromRegExp(terms); | ||
const result = !terms?.length ? '' : `${separator}__name__=~"${regex}"`; | ||
return result; | ||
} | ||
default: | ||
return value; | ||
} | ||
}, | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just not sure I understand what is going on here.
I think it would be easier to just represent / store it as a search string, but parse it into terms and build the regex on the fly (in a function called from _refreshMetricNames), think representing this as a new type of variable with custom formatter could just be complicating things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be the case. It was an interesting exploration into making variables, but on the most part things are much cleaner with a metricSearch
state property on DataTrail.
Here is the new implementation of the content you highlighted here;
https://github.com/grafana/grafana/pull/87884/files#r1626618400
private async _refreshMetricNames() { | ||
const trail = getTrailFor(this); | ||
const timeRange: RawTimeRange | undefined = trail.state.$timeRange?.state; | ||
|
||
if (!timeRange) { | ||
return; | ||
} | ||
|
||
const matchTerms = []; | ||
|
||
const filtersVar = sceneGraph.lookupVariable(VAR_FILTERS, this); | ||
const hasFilters = filtersVar instanceof AdHocFiltersVariable && filtersVar.getValue()?.valueOf(); | ||
if (hasFilters) { | ||
matchTerms.push(sceneGraph.interpolate(trail, '${filters}')); | ||
} | ||
|
||
const metricSearchRegex = createPromRegExp(trail.state.metricSearch); | ||
if (metricSearchRegex) { | ||
matchTerms.push(`__name__=~"${metricSearchRegex}"`); | ||
} | ||
|
||
const match = `{${matchTerms.join(',')}}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how we build the match
part of the query now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice , I think this looking good now m, wdyt?
@@ -223,6 +229,8 @@ export class DataTrail extends SceneObjectBase<DataTrailState> { | |||
stateUpdate.topScene = new MetricSelectScene({}); | |||
} | |||
|
|||
stateUpdate.metricSearch = values.metricSearch?.toString() || undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This you need the same logic as above (only set this if it's a string or clear it if it's null )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's due to a strange behavior of url sync , values are only passed to this function if the URL values differ from state (so if they are the same but metric is different then this will be called with values having metric value but not metricSearch). Something I have been wanting to change but not come up with a solution for yet grafana/scenes#551
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This you need the same logic as above (only set this if it's a string or clear it if it's null )
Ok.
Fixed in 3f5905f
1af5712
to
9b64f41
Compare
* main: datatrails: persist search terms and use them to reduce retrieved metric names (#87884) CI: Make pkg/build its own module, remove unused Grafana modules in go.mo… (#89243) Fix typo in dashboard-variables destinations and also use Grafana Cloud docs if they exist (#89244) Tooltip: Add tooltip support to Histogram (#89196) Use ref URIs with the reference style links (#89204) Move RecentlyDeleted into browse-dashboards (#89214) Datasource Config: Return error object on failed updates (#88661)
We believe this change is triggering 414 Request-URI Too Large errors due to very long Prometheus request URLs with "match" request parameter introduced by this change, see #90452. Assuming this is indeed the issue, is there a way to opt out of the functionality introduced by this change, or any other workaround? Thanks in advance. |
Hi @darrenjaneczek, any chance you could comment on the 414 Request-URI Too Large issue? This is currently blocking us from upgrading to Grafana 11.x and may be affecting anyone who uses Prometheus datasources with large number of metrics. I can help figure out more detailed repro steps if needed. Thanks in advance! |
What is this feature?
Creates a metric search terms variable.
This variable is stored as a URL parameter, and sent to the prometheus request for metric names.
A limit is added to the prometheus request to prevent an excessive response of metric names, which could lead to a time out. The user is encouraged to add adhoc filters or additional search terms to reduce the number of results returned.
Why do we need this feature?
Three features:
Limitations:
match[]
parameter excludes "lookahead". Instead, we construct a list of expressions out of the search terms, and requestsn
of those terms to be present in the metric name (where n is the number of search terms). This will result in false positiives, such as "go hello" matching "go_gone" since "go" appears twice. The same javascript regular expression is used as before, which would filter out "go_gone" since it lacks "hello". Unfortunately, the limit will include these false positives.Special notes for your reviewer:
Please check that: