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

Add prometheus exporter #10767

Merged
merged 21 commits into from
Jun 3, 2021
Merged

Add prometheus exporter #10767

merged 21 commits into from
Jun 3, 2021

Conversation

bernd
Copy link
Member

@bernd bernd commented Jun 1, 2021

This implements a prometheus exporter that maps the internal dropwizard
metrics to prometheus compatible metric names and labels.

The metric mapping is configurable via config files.

Fixes #9914

TODO

  • Add some tests
  • Register our default exporter port in the prometheus default port registry
  • Check for TODOs and commented code
  • Check that the metrics filter works correctly
  • Add error handling and constant for the built-in resource file
  • Add HTTP endpoint that returns a JSON payload for the file based discovery in prometheus
  • Test with a real prometheus

bernd added 4 commits June 1, 2021 22:25
This implements a prometheus exporter that maps the internal dropwizard
metrics to prometheus compatible metric names and labels.

The metric mapping is configurable via config files.
@bernd bernd requested a review from danotorrey June 1, 2021 20:36
@danotorrey danotorrey self-assigned this Jun 1, 2021
Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

@bernd I really like the approach and design here. It's a nice improvement, which allows for configurability, and improves the definition of the mappings (both core and custom). I really did not like how the mappings definitions were explicitly defined in Java code in the first implementation. YML works very well as far as I can see here.

I still need to do more review and testing, but it does appear to work. I will take one more pass through with deeper testing. I left a few questions from the first look.

misc/graylog.conf Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
misc/graylog.conf Show resolved Hide resolved
Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

Well done! I tested this with a Prometheus instance running, and it works. The live refresh of the core/custom mappings is really cool. I am sure users will love that. It also looks like the existing mappings and label are being processed correctly and appear correct in Prometheus as they did with the original Cloud implementation.

From testing, I found a few items that I gave some feedback on. Once this is ready for review, I will do one last final pass through the code.

@bernd bernd marked this pull request as ready for review June 3, 2021 15:07
@bernd bernd requested a review from danotorrey June 3, 2021 15:07
Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks a lot for the fixes. All tests successfully. I am merging this now.

I noticed one interesting aspect worth mentioning. Now that the defaults are not specified for the file, the edge case I mentioned is probably extremely hard to hit. Without defaults, the FilePathReadableValidator.class config validator seems to prevent nonexistent files from being specified on startup - effectively preventing the problematic situation. I think the defaults were exempt from the validation :D It's fine as is now since there's extra protection around it.

success: "false"
type: "graylog"

- metric_name: "input-buffer-thread-factory"

Choose a reason for hiding this comment

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

Shouldn't these be underscores instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fungusakafungus You are absolutely right. Thank you very much for seeing this. #10904 has been created, and we will fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been fixed in #10905 and will be included in the 4.1 release.

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.

Add prometheus metrics endpoint with proper tag support
3 participants