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

Graphite: Process multiple queries to Graphite plugin #59608

Merged
merged 9 commits into from Dec 1, 2022

Conversation

mmandrus
Copy link
Contributor

@mmandrus mmandrus commented Nov 30, 2022

What is this feature?

Updates the Graphite datasource plugin to handle multiple queries, instead of just the last one in the list. It aliases each query in order to correlate queries with results.

Why do we need this feature?

The current behavior is causing issues in public dashboards, which we had previously not seen because the front end sends graphite queries to Graphite through the proxy instead of through /ds/query.

Which issue(s) does this PR fix?:

Fixes #59601

Special notes for your reviewer:

There is some less-than-ideal code in here to handle associating graphite query results with requests, but based on my research, it would not be safe to rely on result order aligning with query order. Using aliases seemed to be the safest approach.

An alternative would have been to send off multiple different queries to Graphite, but this seemed preferred to minimize the impact of errors.

Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

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

LGTM
It would be nice if you could provide unit tests for this.

@mmandrus mmandrus added this to the 9.3.2 milestone Dec 1, 2022
@mmandrus mmandrus added add to changelog no-backport Skip backport of PR labels Dec 1, 2022
@mmandrus mmandrus changed the title Graphite: Process multiple queries to Graphite Graphite: Process multiple queries to Graphite plugin Dec 1, 2022
Copy link
Contributor

@bohandley bohandley left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmandrus mmandrus merged commit 0c560b8 into main Dec 1, 2022
@mmandrus mmandrus deleted the mmandrus/fix-multiple-graphite-queries branch December 1, 2022 23:05
@bobemoe
Copy link
Contributor

bobemoe commented Dec 10, 2022

Hi, thanks for the fix. Unfortunately looks like this may have now broken the wildcard queries. While I can now confirm this fixes the original issue, now any wildcard metrics are only showing the last match of the set. Shall I open new issue with screenshots etc?

@bobemoe
Copy link
Contributor

bobemoe commented Dec 10, 2022

Just to note wildcards were working in the original issue and were the only way to get multiple lines. This is now kind of the reverse!

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

Successfully merging this pull request may close these issues.

Public dashboard only showing one query per panel
5 participants