-
Notifications
You must be signed in to change notification settings - Fork 61
fix(config) - added missing opentelemetry options #1915
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
fix(config) - added missing opentelemetry options #1915
Conversation
…m default 9464 to 9090, and overriden exporter format
@@ -333,6 +334,9 @@ controlplane: | |||
opentelemetry: |- | |||
otel.javaagent.enabled=false | |||
otel.javaagent.debug=false | |||
# By default prometheus uses port 9464 |
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.
One slight proposal on this comment and the same below, @kkotowiczz. I do not understand the point. Is it, that this config is only needed, because otel and prometheus are not aligned concerning port? Please adapt the comment in a way, that describes the issue better. Thanks!
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.
If it's just a configuration issue why is this change needed to happen here? tbh I don't see why we should have a particular exporter configured by default when that should be done by the user (in fact, the java agent is disabled by default, see line 335)
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.
Ok, so it seems then like #1905 it's not a bug, because these are the only changes needed to scrap metrics with prometheus.
So I don't know how you proceed in similar cases.
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.
Maybe we can just provide documentation to tell users how they can have the /metrics
endpoint exposed
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.
Closing PR then
|
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.
Discussion on the PR aside, the PR should come with an automated way to test it, or a reasonable motivation if that's not possible
@@ -333,6 +334,9 @@ controlplane: | |||
opentelemetry: |- | |||
otel.javaagent.enabled=false | |||
otel.javaagent.debug=false | |||
# By default prometheus uses port 9464 |
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.
If it's just a configuration issue why is this change needed to happen here? tbh I don't see why we should have a particular exporter configured by default when that should be done by the user (in fact, the java agent is disabled by default, see line 335)
@kkotowiczz the documentation could have been done in this PR :) |
WHAT
remapped port from default 9464 to 9090, and overriden exporter format
WHY
In order to be able to scrap metrics with prometheus, when otel javaagent is enabled
FURTHER NOTES
N/A
Closes #1905