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

prometheus metric_version = 2 & make scrape url as a configurable tag #5767

Merged
merged 9 commits into from
Nov 21, 2019

Conversation

vishiy
Copy link
Contributor

@vishiy vishiy commented Apr 25, 2019

Required for all PRs:

  • [*] Signed CLA.
  • [*] Associated README.md updated.
  • [* ] Has appropriate unit tests.

#4415
#5689
#5798

  • Ensured the following -
  • make check
  • make test
  • all prometheus input tests work (existing & new tests)

@glinton glinton added area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Apr 25, 2019
@danielnelson danielnelson added this to the 1.11.0 milestone Apr 26, 2019
@vishiy vishiy changed the title prometheus metric_version = 2 prometheus metric_version = 2 & make scrape url as a configurable tag May 13, 2019
@vishiy
Copy link
Contributor Author

vishiy commented May 13, 2019

Fixed the below issue as well as part of this same outstanding PR. (updated PR title to reflect the same)

#5798

@danielnelson danielnelson modified the milestones: 1.11.0, 1.12.0 Jun 5, 2019
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

I think I need to work on #3670 before merging, and specifically a way to have per field metric types. This will allow us to parse:

# HELP haproxy_up Was the last scrape of haproxy successful.
# TYPE haproxy_up gauge
haproxy_up 1

# HELP haproxy_left Was the last scrape of haproxy successful.
# TYPE haproxy_left counter
haproxy_left 1

# HELP haproxy_down Was the last scrape of haproxy successful.
# TYPE haproxy_down untyped
haproxy_down 1

And have them show up on a single line:

prometheus haproxy_up=1,haproxy_left=1,haproxy_down=1

But still convert them back to typed metrics in the prometheus output.

plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
@vishiy
Copy link
Contributor Author

vishiy commented Jun 6, 2019

I think I need to work on #3670 before merging, and specifically a way to have per field metric types. This will allow us to parse:

# HELP haproxy_up Was the last scrape of haproxy successful.
# TYPE haproxy_up gauge
haproxy_up 1

# HELP haproxy_left Was the last scrape of haproxy successful.
# TYPE haproxy_left counter
haproxy_left 1

# HELP haproxy_down Was the last scrape of haproxy successful.
# TYPE haproxy_down untyped
haproxy_down 1

And have them show up on a single line:

prometheus haproxy_up=1,haproxy_left=1,haproxy_down=1

But still convert them back to typed metrics in the prometheus output.

@danielnelson - when is 1.12.0 expected to ship?

@danielnelson
Copy link
Contributor

when is 1.12.0 expected to ship?

It will be around the beginning of September, sorry.

@Esity
Copy link

Esity commented Jun 6, 2019

@danielnelson We need to think of a long term plan for these types of situations. Setting a metric version to 1,2,3 is not sustainable for large deployments and makes upgrading difficult

@danielnelson
Copy link
Contributor

@Esity I agree it is quite hard to make an upgrade even with these options. Do you have an idea of what could make schema changes go smoother to help get us thinking?

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

I'm going to push #3670 off for now so we can for sure get this in for 1.13.0, there are just a few minor changes I'd like made first:

plugins/inputs/prometheus/parser.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
--test output --
go test -short ./plugins/inputs/prometheus/...
ok      github.com/influxdata/telegraf/plugins/inputs/prometheus        0.034s
@vishiy
Copy link
Contributor Author

vishiy commented Nov 4, 2019

@danielnelson - i fixed all the changes requested a few weeks back. Do you know when this is expected to be merged & shipped ? Thank you!

@danielnelson
Copy link
Contributor

@vishiy Sorry, I've just been out of town, will take a second look once I get situated. Expecting to merge for 1.13.

@vishiy
Copy link
Contributor Author

vishiy commented Nov 5, 2019

@vishiy Sorry, I've just been out of town, will take a second look once I get situated. Expecting to merge for 1.13.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants