-
Notifications
You must be signed in to change notification settings - Fork 976
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 Exemplars support to Prometheus 1.x #4867
Add Exemplars support to Prometheus 1.x #4867
Conversation
be40a4c
to
950c8f4
Compare
*/ | ||
public PrometheusMeterRegistry(PrometheusConfig config, PrometheusRegistry registry, Clock clock, | ||
@Nullable ExemplarSampler exemplarSampler) { | ||
@Nullable ExemplarSamplerFactory exemplarSamplerFactory) { |
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 think an implementation of SpanContext
is what we really need from users. Maybe it would be better to expose that in the API instead. I'm struggling to think of when a user would need to provide a custom implementation of ExemplarSamplerFactory
(other than to pass a SpanContext
).
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.
The advantage of providing an ExemplarSamplerFactory
is that users can control sampling behavior, i.e.: tweak sampling config or completely redefine its logic.
We provided this possibility with the 0.x client (since we let them inject ExemplarSampler
) and I remember users overriding it to remove exemplars on _count
.
Maybe it can also help testing since right now you need to wait 90ms between recordings, otherwise no Exemplar will be sampler, with a custom sampler or tweaking with sampling config, this can be avoided.
I'm ok replacing it to SpanContext
since it is simpler and if users will ask for it we can add an extra ctor.
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 think if we make sure the Prometheus client can be configured via its standard way (see other comment thread), users will be able to control exemplar sampling interval and exemplars on count etc without us exposing a separate interface or accepting a properties object, or am I still missing something?
If I'm not missing anything, I think we can make these new classes not public to limit new API we need to maintain. It'd be great to get feedback from users on if the API we're exposing makes sense for their needs or not. Unfortunately we won't have much time since this won't make it in until the RC.
public class DefaultExemplarSamplerFactory implements ExemplarSamplerFactory { | ||
|
||
// TODO: Should we expose ExemplarsProperties and let users inject it through a ctor? | ||
private final ExemplarsProperties properties = ExemplarsProperties.builder().build(); |
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.
Since we aren't implementing the behavior controlled by this, I think it's okay for users to rely on the configuration methods documented by the Prometheus client: https://prometheus.github.io/client_java/config/config/
As long as our usage allows these methods to work, then I don't think we need to expose something specifically.
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.
We don't allow changing these defaults and no one asked for this for the old client either.
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 looked into the implementation a bit. I assumed that using the builder would load the values from the locations specified in the documentation and use those unless programmatically overwritten. That's not the case. To get that sort of behavior, we should be using PrometheusPropertiesLoader
, I think.
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.
PrometheusProperties
keeps a singleton instance created by PrometheusPropertiesLoader
and it's get
method returns this instance so using that should be as simple as:
private static final ExemplarsProperties EXEMPLARS_PROPERTIES = PrometheusProperties.get().getExemplarProperties();
and
private static final ExporterProperties EXPORTER_PROPERTIES = PrometheusProperties.get().getExporterProperties();
But to me the question is more like: should we really do this and depend on PrometheusProperties? Or instead we should expose these properties (if we want to) in PrometheusConfig
? In the case of ExemplarsProperties
this might make some sense since we are using the defaults and this let the users override them (though it would be a valid question why they cannot override it through PrometheusConfig
).
But in the case of ExporterProperties
we are not using the default (exemplarsOnAllMetricTypes
is false by default) so either we use the defaults and drop the extra exemplars by default or figure out how to let the users override this by using the prometheus config loading mechanism.
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 did this here: 228d859
With this, you can override ExemplarsProperties
but not ExporterProperties
.
Issues: 1. prometheus-metrics-tracer-initializer is needed: We would only need SpanContext from prometheus-metrics-tracer-common but ExemplarSampler imports SpanContextSupplier so that is also needed from prometheus-metrics-tracer-initializer. 2. Testing is not easy because of the rate limiter since the Prometheus Java Client does not have a clock abstraction that we could mock/fake.
Instead of using 1.0 as the value of the Exemplar for _count, we use the original value of it (recorded latency or value).
950c8f4
to
62f8f38
Compare
f0b1c5e
to
1add599
Compare
Also make ExemplarSamplerFactory and DefaultExemplarSamplerFactory package-private. And make ExporterProperties and ExpositionFormats non-static.
Issues:
prometheus-metrics-tracer-initializer is needed:
We would only need SpanContext from prometheus-metrics-tracer-common
but ExemplarSampler imports SpanContextSupplier so that is also needed
from prometheus-metrics-tracer-initializer.
Testing is not easy because of the rate limiter since the Prometheus
Java Client does not have a clock abstraction that we could mock/fake.