-
Notifications
You must be signed in to change notification settings - Fork 180
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 support for jmx metrics and prometheus exporter #182
Conversation
Added the extra services to the CI, but not sure if that's the best way. |
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.
Looking good, thanks for adding tests! If you'll address all comments, please squash commits, keeping logical changes separate:
- bumping version
- adding new properties
I dont have time to really review but just wanted to mention that this should all be optional and disabled by default. |
4ca2d58
to
e665f52
Compare
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.
Almost there
I realized I was trying to modify the
|
By default, the namespace is random. If you specify a namespace, you're supposed to clean it up manually. But we certainly could ignore the existing namespace and the secret, since it does make sense to avoid using a random namespace when debugging issues locally. Thanks for being patient with all the issues here. |
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.
Two last comments, squash commits and I can merge this.
@kempspo I applied some minor changes and opened #183, preserving your authorship of commits. I hope you don't mind. I rewrote the tests in Python. I think they didn't work, the test container might have completed successfully even if the commands inside failed. Took me a bit to get it right, that's why I know it works now. BTW I removed the |
BTW I could have pushed my changes here if you've opened the PR from any other branch than |
No worries. Those changes make sense to me. Yeah I realized just now that I've been working on |
No description provided.