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

Alerting: Add ability to include aliases with hyphen in InfluxDB #32224

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Alerting: Add ability to include aliases with hyphen in InfluxDB #32224

merged 3 commits into from
Mar 23, 2021

Conversation

dsotirakis
Copy link
Contributor

@dsotirakis dsotirakis commented Mar 22, 2021 •
edited
Loading

What this PR does / why we need it:

Tiny fix, to make InfluxDB metrics include hyphens (-) in their names when aliasing.

Which issue(s) this PR fixes:

Fixes #31284

Special notes for your reviewer:

Steps:

  • Needs an InfluxDB datasource running.
  • Used basic cpu exporter - exports cpu and host metrics.
  • Tweaked metrics locally to transform cpu and host to cpu-a and host-a to reproduce.

Before:

image

After:

image

@dsotirakis dsotirakis added area/alerting/notifications Issues when sending alert notifications type/bug type/chore labels Mar 22, 2021
@dsotirakis dsotirakis force-pushed the fix-regex-influxdb-response-parser branch from e489c64 to 439ba3f Compare March 22, 2021 16:24
@dsotirakis dsotirakis force-pushed the fix-regex-influxdb-response-parser branch from 439ba3f to 9fd63d2 Compare March 23, 2021 07:13
@dsotirakis dsotirakis requested review from marefr and wbrowne March 23, 2021 07:18
@dsotirakis dsotirakis marked this pull request as ready for review March 23, 2021 07:18
@dsotirakis dsotirakis requested a review from a team as a code owner March 23, 2021 07:18
@dsotirakis dsotirakis changed the title Notifications: InfluxDB - fix regex to include metrics with hyphen Notifications: InfluxDB - Fix regex to include metrics with hyphen in aliases Mar 23, 2021
@dsotirakis dsotirakis requested review from a team and ifrost and removed request for wbrowne, marefr and a team March 23, 2021 07:34
@dsotirakis dsotirakis added this to the 7.5.0 milestone Mar 23, 2021
marefr
marefr approved these changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

There's some room for improvement of the tests, but not a blocker for this PR

  • Convert to Go standard tests
  • Seems like table-driven tests could be a good fit for some of the tests

@dsotirakis dsotirakis merged commit fdaac2b into master Mar 23, 2021
@dsotirakis dsotirakis deleted the fix-regex-influxdb-response-parser branch March 23, 2021 11:14
@dsotirakis
Copy link
Contributor Author

LGTM

There's some room for improvement of the tests, but not a blocker for this PR

  • Convert to Go standard tests
  • Seems like table-driven tests could be a good fit for some of the tests

Agree @marefr, should we create a broader ticket to start do some refactoring?

@marefr
Copy link
Contributor

marefr commented Mar 23, 2021

@dsotirakis Our approach is to try and fix things as we change code. For this one you can probably create an issue if that helps.

Note that this has milestone 7.5.0 and needs to be backported into v7.5.x branch for it to actually be included in the release build. Please add backport label

@dsotirakis dsotirakis added the old backport v7.5.x Mark PR for automatic backport to v7.5.x label Mar 23, 2021
@marefr
Copy link
Contributor

marefr commented Mar 23, 2021

You probably want to add "add to changelog" label so that it show up in the changelog for the release. Title can be improved a bit I think. We normally uses the alerting tag for the title, see for example https://github.com/grafana/grafana/blob/master/CHANGELOG.md#features-and-enhancements-1

@grafanabot grafanabot mentioned this pull request Mar 23, 2021
Merged
grafanabot pushed a commit that referenced this pull request Mar 23, 2021
@dsotirakis @grafanabot
Notifications: InfluxDB - Fix regex to include metrics with hyphen in…
f2791cf 
… aliases (#32224)

* Notifications: InfluxDB - fix regex to include metrics with hyphen

* Add hyphen check in tests

(cherry picked from commit fdaac2b)
@dsotirakis dsotirakis changed the title Notifications: InfluxDB - Fix regex to include metrics with hyphen in aliases Alerting: Add ability to include aliases with hyphen in InfluxDB Mar 23, 2021
dsotirakis pushed a commit that referenced this pull request Mar 23, 2021
@grafanabot
Notifications: InfluxDB - Fix regex to include metrics with hyphen in…
80a4d42 
… aliases (#32224) (#32262)

* Notifications: InfluxDB - fix regex to include metrics with hyphen

* Add hyphen check in tests

(cherry picked from commit fdaac2b)
ryantxu pushed a commit that referenced this pull request Mar 24, 2021
@dsotirakis @ryantxu
Notifications: InfluxDB - Fix regex to include metrics with hyphen in…
016e682 
… aliases (#32224)

* Notifications: InfluxDB - fix regex to include metrics with hyphen

* Add hyphen check in tests
@ifrost ifrost requested review from gabor and removed request for gabor March 24, 2021 07:29
@dsotirakis dsotirakis mentioned this pull request Mar 24, 2021
Merged
ryantxu pushed a commit that referenced this pull request Mar 30, 2021
@dsotirakis @ryantxu
Notifications: InfluxDB - Fix regex to include metrics with hyphen in…
77b3b87 
… aliases (#32224)

* Notifications: InfluxDB - fix regex to include metrics with hyphen

* Add hyphen check in tests
ryantxu pushed a commit to dejapong/grafana that referenced this pull request Mar 30, 2021
@dsotirakis @ryantxu
Notifications: InfluxDB - Fix regex to include metrics with hyphen in…
d9490e2 
… aliases (grafana#32224)

* Notifications: InfluxDB - fix regex to include metrics with hyphen

* Add hyphen check in tests
@dsotirakis dsotirakis mentioned this pull request Mar 30, 2021
Merged
@kaydelaney kaydelaney mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marefr marefr marefr approved these changes

@ifrost ifrost Awaiting requested review from ifrost

Assignees
No one assigned
Labels
add to changelog area/alerting/notifications Issues when sending alert notifications area/backend old backport v7.5.x Mark PR for automatic backport to v7.5.x type/bug type/chore
Projects
None yet
Milestone
7.5.0
Development

Successfully merging this pull request may close these issues.

Slack Notification (Alert) doesn't resolve tag value if the tag key contains hyphen (-)
3 participants
@dsotirakis @marefr @grafanabot