-
Notifications
You must be signed in to change notification settings - Fork 45
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
Yabeda integration for prom metrics #1241
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're almost there! Left you some comments to trim a bit what's not used in RMT.
…tings.dig for metrics.enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
I'm not really sure what is the scope. The metric export now most information, I hope this is fine.
How I reviewed this merge request:
- Checked the code changes make sense to me
- Ran the branch and checked
/metrics
is populated (also in error case)
As always, if you think I missed something I should test, please let me know!🚀
cheers,
This reverts commit 63119c3.
Error gets built automagically by rspec, no need to { error: <string> }.to_json
Expect was too granular and specific
50df6dc
to
384b00b
Compare
Description
How to test
Start the rails server:
> bin/rails s
On a separate console, run:
> curl localhost:4224/metrics
Should see a text with metrics and values
Change Type
Please select the correct option.
Checklist
Please check off each item if the requirement is met.
MANUAL.md
file with any changes to the user experience.package/obs/rmt-server.changes
.Review
Please check out our review guidelines
and get in touch with the author to get a shared understanding of the change.