-
Notifications
You must be signed in to change notification settings - Fork 1
feat!: update all bundled metrics configs to export Prometheus metrics #151
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
9d1d33e
to
6cdec3a
Compare
06bbca3
to
2b2f7b0
Compare
why is this merging into OB-38651-config-flag instead of just |
2b2f7b0
to
b2d5253
Compare
This was just how I had organized my branches locally, since I assumed the other change would land first. I was doing more extensive testing on this branch and preferred to test with all chagnes. With the other PR landed, I rebased this branch on top of |
exporters: [otlphttp/observe] | ||
exporters: [prometheusremotewrite/observe] | ||
|
||
metrics/agent-internal-count: |
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.
@obs-gh-alexlew mind taking another look at this part? This is the only change since you last reviewed (directly copy/pasted it to the 3 other packages). This handles converting the count deltas to cumulative metrics.
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.
any reason this has to be a separate pipeline? does this processor do anything to non-delta histogram metrics? also any downsides/footguns to be aware of with this processor?
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 double checked the code and it doesn't do anything to cumulative metrics. One possible reason to separate this is that this processor emits internal telemetry data: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.114.0/processor/deltatocumulativeprocessor/documentation.md but I think going back to a single pipeline makes sense anyway. I'll make that change.
I couldn't find any footguns. I do know that we use it for customer deployments, and also found that it is explicitly recommended in the prometheus docs: https://prometheus.io/docs/guides/opentelemetry/#delta-temporality
609d1ab
to
38b99c2
Compare
#151) ### Description OB-40055 Update all bundled metrics configs to export Prometheus metrics.
#151) ### Description OB-40055 Update all bundled metrics configs to export Prometheus metrics.
#151) ### Description OB-40055 Update all bundled metrics configs to export Prometheus metrics.
Description
OB-40055 Update all bundled metrics configs to export Prometheus metrics.
I tested this with a direct write datastream and the metrics look good! I would love some advice on specific things to manually test, as this change won't be caught at all by our automated testing suite.
Checklist