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.

Sign up for GitHub

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

Jump to bottom

Graphite: Add error information to graphite queries tracing #55249

Merged
merged 1 commit into from Oct 4, 2022
Merged

Graphite: Add error information to graphite queries tracing #55249

merged 1 commit into from Oct 4, 2022

Conversation

jesusvazquez
Copy link
Member

What this PR does / why we need it:

This PR extends graphite QueryData() instrumentation to include in the traces information about possible errors.

  • I've added an attribute about the graphite response code as well as records for errors if there are any.

The reason why I added this information to the traces is that we're seeing some alerts failing in Grafana with no data / value and we don't know the reason behind it. Because of this we'd like to see in the traces that graphite's response or parsing it is not the issue.

@jesusvazquez
Add error information to graphite queries tracing
8eb43e4 
This commit extends graphite QueryData() instrumentation to include in
the traces information about possible errors.

I've added an attribute about the graphite response code as well as
records for errors if there are any.
@CLAassistant
Copy link

CLAassistant commented Sep 15, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33627

@jesusvazquez jesusvazquez marked this pull request as ready for review September 15, 2022 15:17
@jesusvazquez jesusvazquez requested a review from a team as a code owner September 15, 2022 15:17
@gtk-grafana
Copy link
Contributor

This looks good to me! @itsmylife @kylebrandt Can one of y'all take a look at this when you've got a minute?

itsmylife
itsmylife approved these changes Sep 21, 2022
View reviewed changes
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

@itsmylife
Copy link
Contributor

itsmylife commented Sep 21, 2022 •
edited

@gtk-grafana @jesusvazquez @kylebrandt should we backport this to v9.1.x? And @jesusvazquez which milestone fits better for this one? Do you have any suggestions since you are having the issue?

@gtk-grafana
Copy link
Contributor

It might be nice to backport if this is meant to debug existing issues on running grafana instances.

@jesusvazquez
Copy link
Member Author

if this is meant to debug existing issues on running grafana instances

This is the main reason why we're improving the tracing in this PR but I'm not sure they are Grafana issues yet. However it is great to have information in the traces about what graphite responses as well as tracing the error if any.

How can this be backported? Also I'm not sure how your milestones work so I need a bit of help there.

@itsmylife itsmylife added backport v9.0.x backport v9.1.x Bot will automatically open backport PR labels Sep 22, 2022
@itsmylife itsmylife added this to the 9.1.7 milestone Sep 22, 2022
@itsmylife itsmylife removed the backport v9.1.x Bot will automatically open backport PR label Sep 22, 2022
@itsmylife
Copy link
Contributor

if this is meant to debug existing issues on running grafana instances

This is the main reason why we're improving the tracing in this PR but I'm not sure they are Grafana issues yet. However it is great to have information in the traces about what graphite responses as well as tracing the error if any.

How can this be backported? Also I'm not sure how your milestones work so I need a bit of help there.

I set the milestone as v9.1.7. And backport to v9.0.x. So this will be in the release v9.1.7 which should be the next possible release. And it will be backported to the v9.0.x. So all v9.x versions will have it. I think now everything is fine :)

@ying-jeanne ying-jeanne merged commit cb99b94 into main Oct 4, 2022
@ying-jeanne ying-jeanne deleted the jvp/add-span-information-about-error-to-graphite-queries branch October 4, 2022 08:30
@grafanabot grafanabot mentioned this pull request Oct 4, 2022
Merged
grafanabot pushed a commit that referenced this pull request Oct 4, 2022
@jesusvazquez @grafanabot
Add error information to graphite queries tracing (#55249)
2486a8e 
This commit extends graphite QueryData() instrumentation to include in
the traces information about possible errors.

I've added an attribute about the graphite response code as well as
records for errors if there are any.

(cherry picked from commit cb99b94)
ying-jeanne pushed a commit that referenced this pull request Oct 4, 2022
@grafanabot @jesusvazquez
Add error information to graphite queries tracing (#55249) (#56246)
50d81e4 
This commit extends graphite QueryData() instrumentation to include in
the traces information about possible errors.

I've added an attribute about the graphite response code as well as
records for errors if there are any.

(cherry picked from commit cb99b94)

Co-authored-by: Jesus Vazquez <jesusvazquez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsmylife itsmylife itsmylife approved these changes

Assignees
No one assigned
Labels
add to changelog area/backend backport v9.0.x datasource/Graphite enterprise-failed
Projects
None yet
Milestone
9.1.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jesusvazquez @CLAassistant @grafanabot @gtk-grafana @itsmylife @ying-jeanne