-
Notifications
You must be signed in to change notification settings - Fork 772
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 exemplar support to Prometheus exporter #5929
base: main
Are you sure you want to change the base?
Conversation
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusSerializerExt.cs
Show resolved
Hide resolved
I think you can enable it by default, given Exemplars itself is off-by-default in the SDK! So if prometheus exporter is exporting exemplars, users has already made an explicit decision to enable it at the sdk. |
@saul Can you sign the CLA part, as it is a mandatory pre-req for accepting any contributions to OpenTelemetry. If you are working on behalf on an employer, you may want to consult your employer before CLA. (Note: This is a one time requirement only) |
a609c15
to
b412857
Compare
d6e35f6
to
bfedbfc
Compare
bfedbfc
to
8550c31
Compare
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Hi @cijothomas the CLA has been signed and this is ready for review when you have time. Thanks |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
@robertcoltheart Tagging you here, to see if you have some bandwidth to help review this PR, given you have made several contributions to the Prometheus Exporter! |
I am not sure of the reasoning for this either...You can put a TODO in the code and state this is not implemented, and we can follow up with the spec. |
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.
From a technical perspective, this seems fine. I can't comment on whether this meets the Otel specification to a degree that is satisfactory, or whether there should be additional things added. Ie. I haven't done a mapping to see if there are any gaps between the spec and this PR.
return WriteExemplar(buffer, cursor, maxExemplar, metric.Name, isLong); | ||
} | ||
|
||
private static int WriteExemplar(byte[] buffer, int cursor, in Exemplar exemplar, string metricName, bool isLong) |
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.
Should this go into PrometheusSerializer.cs
alongside the rest of the byte-writing code for metrics?
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Fixes #3449
Design discussion issue - not found
Changes
Adds support for exporting histogram exemplars to OpenTelemetry.Exporter.Prometheus.AspNetCore and OpenTelemetry.Exporter.Prometheus.HttpListener. There are a couple of aspects that I'd appreciate some guidance on:
/metrics
endpoint? Or should the exemplars be persistent in that once a bucket has an exemplar, it will always be returned by the/metrics
endpoint until it is replaced by another exemplar.PrometheusAspNetCoreOptions
to enable exemplars, with the default being disabled (i.e. current behaviour)Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changesPrometheusAspNetCoreOptions
option