Skip to content
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

Support $rate_interval when parsing Query during dashboards analysis #6657

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

wilfriedroset
Copy link
Collaborator

@wilfriedroset wilfriedroset commented Nov 14, 2023

Grafana support several Global variables but also allow users to define an interval templated variable. When using mimirtool on such case we might have the following error:

"query=histogram_quantile(0.70, sum by (le) (rate(rpc_server_duration_milliseconds_bucket{rpc_method=~\"$gRPC_method\", job=\"$job\"}[$rate_interval]))): 1:124: parse error: missing unit character in duration",

This commit add support for more user define variables matching the global ones.

See: https://grafana.com/docs/grafana/latest/dashboards/variables/add-template-variables/#global-variables

We have the following global variables that are out of the scope of dashboards analysis:

  • $__dashboard
  • $__from and $__to
  • $__name
  • $__org
  • $__user
  • $timeFilter or $__timeFilter
  • $__timezone

However those are interesting:

  • $__interval --> can be overridden by $interval
  • $__interval_ms --> can be overridden by $interval
  • $__range --> not addressed by this PR, should we?
  • $__rate_interval --> can be overridden by $rate_interval
  • $__rate_interval_ms --> can be overridden by $rate_interval

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@wilfriedroset wilfriedroset requested a review from a team as a code owner November 14, 2023 16:22
@colega
Copy link
Contributor

colega commented Nov 14, 2023

We have an internal tool that does a similar job, and instead of a list of string replaces it just does:

var variableRangeQueryRangeRegex = regexp.MustCompile(`\[\$?\w+?]`)
var variableSubqueryRangeRegex = regexp.MustCompile(`\[\$?\w+:\$?\w+?]`)

func removeVariablesFromRanges(query string) string {
	query = variableRangeQueryRangeRegex.ReplaceAllLiteralString(query, `[5m]`)
	query = variableSubqueryRangeRegex.ReplaceAllLiteralString(query, `[5m:1m]`)
	return query
}

WDYT about applying it here?

@colega
Copy link
Contributor

colega commented Nov 15, 2023

Nice! Would you mind adding a CHANGELOG entry? 🙏 Then we're good to go.

Grafana support several Global variables but also allow users to define
an `interval` templated variable. When using mimirtool on such case we
might have the following error:

```
"query=histogram_quantile(0.70, sum by (le) (rate(rpc_server_duration_milliseconds_bucket{rpc_method=~\"$gRPC_method\", job=\"$job\"}[$rate_interval]))): 1:124: parse error: missing unit character in duration",
```

This commit improve the support variables in ranges.

See: https://grafana.com/docs/grafana/latest/dashboards/variables/add-template-variables/#global-variables

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@colega colega merged commit f72816b into grafana:main Nov 15, 2023
28 checks passed
@wilfriedroset wilfriedroset deleted the mimirtool-rate-interval branch November 15, 2023 10:41
cristiangreco added a commit that referenced this pull request May 6, 2024
When parsing queries from grafana dashboards, use a replacer with a
list of variables rather than a regex matcher.

The regex matcher works ok with ranges and subqueries, but is not
working when the variables are used in other parts of the query (e.g.
with `offset`).

This PR switches back to using a list of variables to be replaced,
partially reverting the changes from #6657, but uses a
`strings.Replacer` for better readability.
colega pushed a commit that referenced this pull request May 6, 2024
* mimirtool: use strings replacer in dashboards queries analyzer

When parsing queries from grafana dashboards, use a replacer with a
list of variables rather than a regex matcher.

The regex matcher works ok with ranges and subqueries, but is not
working when the variables are used in other parts of the query (e.g.
with `offset`).

This PR switches back to using a list of variables to be replaced,
partially reverting the changes from #6657, but uses a
`strings.Replacer` for better readability.

* restore regexps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants