-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[receiver/prometheusreceiver] implement append native histogram #28663
Merged
jpkrohling
merged 47 commits into
open-telemetry:main
from
krajorama:implement-appendhistogram
Apr 9, 2024
Merged
Changes from 43 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
ae09fe9
prometheusreceiver: first step towards being able to append native hi…
krajorama 1dd30ef
Start adding some unit for transaction.go
krajorama 2ab22e6
Add buckets conversion logic
krajorama cb6eee8
Set timestamps and attribues
krajorama 94b13af
Add to adjustmetrics and prefer exponential histograms
krajorama 26ad5e0
Fix lint error
krajorama 7ed1c48
More lint fixes
krajorama 8a20021
Update startTimeMetricAdjuster
krajorama 7ff5be1
Improve comment
krajorama 2c35f51
Add first e2e test
krajorama db01595
Fix and test scrape_classic_histograms scrape option basic handling
krajorama 68614ab
Remove extra logs not needed anymore
krajorama e7334cf
Remove unused import
krajorama 068fa0e
Add unit tests for initial point adjuster on exponential histograms
krajorama 5ff25f4
Handle exponential histogram staleness marker
krajorama 8b41107
Add unit test and fix for staleness marker
krajorama e8e2b8b
Fix heuristics around staleness marker
krajorama f259787
Drop unsupported gauge histograms
krajorama b10838a
Update readme
krajorama ec930f3
Linter induced test fixes
krajorama 9618f5a
Apply suggestions from code review
krajorama 06838f7
Handle float histograms in addExponentialHistogramSeries
krajorama 42bc0c1
Fix consistency in toExponentialHistogramDataPoints
krajorama 5e10890
Fix using Prometheus Config
krajorama 1d36a8e
Add setting ZeroThreshold in exponential histogram
krajorama 27c9f52
Fix: created timestamp was not correctly added for exponential histog…
krajorama 86a73fa
Add feature gate receiver.prometheusreceiver.EnableNativeHistograms
krajorama 5271dbf
Fix typo
krajorama a6b49f2
Merge branch 'main' into implement-appendhistogram
krajorama c4211f9
Run go mod tidy
krajorama d4daf7c
Fix linter issues
krajorama 1e50b6a
Merge branch 'main' into implement-appendhistogram
krajorama 92c2bcc
Enforce scraping classic histograms if native histograms feature is off
krajorama 8d09788
Merge branch 'main' into implement-appendhistogram
krajorama a4695d9
Updates from review comments.
krajorama 8bd610f
Merge branch 'main' into implement-appendhistogram
krajorama c3b2596
Followup merge from main
krajorama 8d33a89
Tweak readmes
krajorama 7ec9dc0
Add check on schema in metrics_receiver_protobuf_test.go
krajorama 0ecd615
Merge branch 'main' into implement-appendhistogram
krajorama c399b73
Adopt 31908
krajorama 90c2810
Fix linting
krajorama 3c5ae66
Merge branch 'main' into implement-appendhistogram
krajorama 27e61c5
Remove unsupported option enable_protobuf_negotiation
krajorama a67e214
Merge branch 'main' into implement-appendhistogram
krajorama 58be349
Merge branch 'main' into implement-appendhistogram
krajorama 79ec8c9
Merge branch 'main' into implement-appendhistogram
dashpole File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
component: prometheusreceiver | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Allows receiving prometheus native histograms | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
issues: [26555] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
- Native histograms are compatible with OTEL exponential histograms. | ||
- The feature can be enabled via the feature gate `receiver.prometheusreceiver.EnableNativeHistograms`. | ||
Run the collector with the command line option `--feature-gates=receiver.prometheusreceiver.EnableNativeHistograms`. | ||
- Currently the feature also requires that targets are scraped via the ProtoBuf format. | ||
To start scraping native histograms, set | ||
`config.global.scrape_protocols` to `[ PrometheusProto, OpenMetricsText1.0.0, OpenMetricsText0.0.1, PrometheusText0.0.4 ]` in the | ||
receiver configuration. This requirement will be lifted once Prometheus can scrape native histograms over text formats. | ||
- For more up to date information see the README.md file of the receiver at | ||
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/prometheusreceiver/README.md#prometheus-native-histograms. | ||
|
||
# If your change doesn't affect end users or the exported elements of any package, | ||
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this something we will eventually enable by default? Will we ever want it always-on? A feature-gate might be better if we want this to eventually always be enabled.
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.
Prometheus will enable two things by default eventually (I'd guess next year):
Actually the text format for native histograms is being worked on currently, so again eventually it will not matter what you set protobuf negotiation to.
Given the current code, to follow Prometheus, the default for
enable_protobuf_negotiation
option should be switched totrue
in otel collector as well when Prometheus makes the change.The real "problem" will be native histograms being on by default. Since for a metric that has both classic and native buckets, we can only scrape one due to the name conflict (both result in the same metric name). So if we start enabling native histograms by default, suddenly the classic buckets would be gone, replaced by exponential buckets.
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, we would want our eventual configuration to only include a knob for switching between fixed-bucket and exponential histogram, right? Do we need to keep config for protobuf negotiation around long-term? It sounds like we essentially just want to keep the default histogram format as fixed-bucket, rather than exponential
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 guess so. The question that comes up for Prometheus is different, because you can have the classic histograms beside the native histograms (as names are different). Still the issue is similar in that if we enable native histograms by default, you suddenly have a breaking change by switching from classic buckets to native buckets. And maybe we don't need an extra setting since you can set
scrape_classic_histograms
in the scrape config. In Prometheus that gets you the classic buckets as well and in otel collector it would get you the classic buckets only. At least in the my current implementation.The whole thing only affects cases where the metric has both classic and native defined.
cc @beorn7
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.
Enabling protobuf negotiation can now be done using the prometheus scrape config. See #30934.
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.
Will wait for PR #30934 and rebase.
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 this can be removed now