-
Notifications
You must be signed in to change notification settings - Fork 616
OTel: Allow users to add metrics exporters. #4189
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
Conversation
@@ -168,14 +168,22 @@ func EnablePatroniMetrics(ctx context.Context, | |||
}, | |||
} | |||
|
|||
// If there are exporters to be added to the metrics pipelines defined | |||
// in the spec, add them to the pipeline. | |||
exporters := []ComponentID{Prometheus} |
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.
So there's no way for a user to remove the Prometheus exporter? (Of course, if they did, we'd maybe also want to change the way we expose the port?)
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.
No, but I'm not opposed to providing a way to remove it. I'm just not sure that we should remove it by default whenever the user provides their own exporters.
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.
I keep thinking about the way we deal with debug for logs as a model, but maybe that's too different?
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.
Yeah feels pretty different to me. The debug exporter is there just so that there is some way to view logs if the user doesn't provide another solution, but the Prometheus exporter is key for integrating into our monitoring stack.
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.
Yeah, agreed -- we can use this PR to add other exporters and maybe another to remove Prometheus/remove the exposed port (if we want to. I can imagine someone who wants their own exporter might not want Prometheus at all).
(Though also I'm still curious if there's other ports to expose or other changes for other metrics exporters, but at least we've covered the major ones, I think.)
Wonder if we could add a metrics exporter stage to the Kuttl test? |
Just did. Please take a look. |
// The queries aren't really needed for this test and sheer number of queries | ||
// would make this file excessively long (and string formatting presented it's | ||
// own formatting headaches), so I am removing them |
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.
💯 for the comment and logic
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Currently, OTel metrics are only exported by the default prometheus exporter that we set up to work with our monitoring stack. Users cannot add their own metrics exporters.
What is the new behavior (if this is a feature change)?
Users can now add their own metrics exporters. The default
prometheus
exporter has a new component ID ofprometheus/cpk-monitoring
to differentiate it from any otherprometheus
exporters that users might add.Other Information: