[chore](prometheusreceiver): use type and unit as identifier in tests #38645
Conversation
This file contains hidden or 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
krajorama
force-pushed
the
promreceiver-identifying
branch
3 times, most recently
from
a217a55 to
09cbd64
Compare
krajorama
changed the title
krajorama
force-pushed
the
promreceiver-identifying
branch
4 times, most recently
from
191b7b5 to
d751e3e
Compare
I'd like to fix a technical dept from open-telemetry#28663 where the Promtheus reveiver didn't allow receiving both classic and native histogram for the same metric name. To test the implementation, the assertions must not assume that the metric name alone is identifying in OpenTelemetry. No new tests added, I just refactored to make name+type+unit the identifier. Also I made it implicit that doCompare fails on metrics that were not expected - excluding the 5 scrape metrics. This means that assertMetricAbsent is not needed anymore. Signed-off-by: György Krajcsovits <[email protected]>
krajorama
force-pushed
the
promreceiver-identifying
branch
from
d751e3e to
0ec4ee9
Compare
|
@Aneurysm9, @dashpole please review as codeowners. |
|
The PR #38652 depends on this. |
krajorama
commented
Mar 19, 2025
| @@ -154,8 +158,9 @@ func verifyRenameMetric(t *testing.T, td *testData, resourceMetrics []pmetric.Re | |||
| compareAttributes(map[string]string{"method": "post", "port": "6381"}), | |||
| }, | |||
| }, | |||
| }), | |||
| assertMetricAbsent("rpc_duration_total"), | |||
There was a problem hiding this comment.
note to reviewers: no longer needed as doCompare does 1-1 check
krajorama
commented
Mar 19, 2025
Comment on lines
-195
to
-197
| assertMetricAbsent("http_go_threads"), | ||
| assertMetricAbsent("http_connected_total"), | ||
| assertMetricAbsent("redis_http_requests_total"), |
There was a problem hiding this comment.
note to reviewers: no longer needed as doCompare does 1-1 check
krajorama
commented
Mar 19, 2025
| []dataPointExpectation{{ | ||
| histogramPointComparator: []histogramPointComparator{ | ||
| compareHistogram(1213, 456, []float64{0.5, 10}, []uint64{789, 222, 202}), | ||
| }, | ||
| }}, | ||
| ), | ||
| // When the native histograms feature is off, no native histograms are scraped. | ||
| assertMetricAbsent("test_native_histogram"), |
There was a problem hiding this comment.
note to reviewers: no longer needed as doCompare does 1-1 check
krajorama
commented
Mar 19, 2025
| []dataPointExpectation{{ | ||
| histogramPointComparator: []histogramPointComparator{ | ||
| compareHistogram(1213, 456, []float64{0.5, 10}, []uint64{789, 222, 202}), | ||
| }, | ||
| }}, | ||
| ), | ||
| // When the native histograms feature is off, no native histograms are scraped. | ||
| assertMetricAbsent("test_native_histogram"), |
There was a problem hiding this comment.
note to reviewers: no longer needed as doCompare does 1-1 check
krajorama
commented
Mar 19, 2025
|
|
||
| func assertMetricAbsent(name string) testExpectation { |
There was a problem hiding this comment.
note to reviewers: no longer needed as doCompare does 1-1 check
This was referenced Mar 23, 2025
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
not stale, waiting for reviewer, bump |
|
@Aneurysm9 @dashpole can you review as codeowners? |
|
@andrzej-stencel, given that the current codeowners are absent. Could @krajorama and I become codeowners for the Prometheus receiver? I can vouch for Krajo. He is a Prometheus maintainer just like I am and surely knows the scraping components. |
ArthurSens
approved these changes
Apr 15, 2025
Comment on lines
-502
to
-504
| func compareMetricUnit(unit string) metricTypeComparator { | ||
| return func(t *testing.T, metric pmetric.Metric) { | ||
| assert.Equal(t, unit, metric.Unit(), "Metric unit does not match") |
There was a problem hiding this comment.
Just wrapping my head around the changes here. compareMetricUnit doesn't exist anymore because unit comparison is being done on line 540, right? Same applies to compareMetricType
There was a problem hiding this comment.
yes, since they are identifying now, it doesn't make sense to check them independent of the name
andrzej-stencel
approved these changes
Apr 16, 2025
This sounds great to me! ❤️ |
6 tasks
akshays-19
pushed a commit
to akshays-19/opentelemetry-collector-contrib
that referenced
this pull request
Apr 23, 2025
…open-telemetry#38645) I'd like to fix a technical dept from open-telemetry#28663 where the Prometheus receiver didn't allow receiving both classic and native histogram for the same metric name. To test the implementation, the assertions must not assume that the metric name alone is identifying in OpenTelemetry. No new tests added, I just refactored to make name+type+unit the identifier. Also I made it implicit that `doCompare` fails on metrics that were not expected - excluding the 5 scrape metrics. This means that `assertMetricAbsent` is not needed anymore. Signed-off-by: György Krajcsovits <[email protected]>
Fiery-Fenix
pushed a commit
to Fiery-Fenix/opentelemetry-collector-contrib
that referenced
this pull request
Apr 24, 2025
…open-telemetry#38645) I'd like to fix a technical dept from open-telemetry#28663 where the Prometheus receiver didn't allow receiving both classic and native histogram for the same metric name. To test the implementation, the assertions must not assume that the metric name alone is identifying in OpenTelemetry. No new tests added, I just refactored to make name+type+unit the identifier. Also I made it implicit that `doCompare` fails on metrics that were not expected - excluding the 5 scrape metrics. This means that `assertMetricAbsent` is not needed anymore. Signed-off-by: György Krajcsovits <[email protected]>
songy23
pushed a commit
that referenced
this pull request
Apr 25, 2025
Depends on #38645 for testing. #### Description This is technical dept from #28663, especially in migration scenarios we want to receive both versions of the same histogram, the classic explicit and exponential bucket histogram. The current internal model of the receiver does not allow this, so I modified the model to not only key metrics by metric family name, but also whether it's an exponential histogram. Some callbacks from Promtheus scrape don't tell us what the metric type is up front. For AppendExemplar to work I made transaction stateful to remember if it appended an exponential histogram before. For Append of staleness marker to work, I modified the algorithm to discover exponential histogram from the cached metadata and the name. Due to prometheus/prometheus#16217 I also had to modify AppendExemplar to detect trying to add exemplars from native histograms even if native histograms are turned off. #### Link to tracking issue Related to #26555 #### Testing Unit test updated. #### Documentation N/A - does not contradict. --------- Signed-off-by: György Krajcsovits <[email protected]>
vincentfree
pushed a commit
to ing-bank/opentelemetry-collector-contrib
that referenced
this pull request
May 6, 2025
…telemetry#39451) Depends on open-telemetry#38645 for testing. #### Description This is technical dept from open-telemetry#28663, especially in migration scenarios we want to receive both versions of the same histogram, the classic explicit and exponential bucket histogram. The current internal model of the receiver does not allow this, so I modified the model to not only key metrics by metric family name, but also whether it's an exponential histogram. Some callbacks from Promtheus scrape don't tell us what the metric type is up front. For AppendExemplar to work I made transaction stateful to remember if it appended an exponential histogram before. For Append of staleness marker to work, I modified the algorithm to discover exponential histogram from the cached metadata and the name. Due to prometheus/prometheus#16217 I also had to modify AppendExemplar to detect trying to add exemplars from native histograms even if native histograms are turned off. #### Link to tracking issue Related to open-telemetry#26555 #### Testing Unit test updated. #### Documentation N/A - does not contradict. --------- Signed-off-by: György Krajcsovits <[email protected]>
vincentfree
pushed a commit
to ing-bank/opentelemetry-collector-contrib
that referenced
this pull request
May 20, 2025
…telemetry#39451) Depends on open-telemetry#38645 for testing. #### Description This is technical dept from open-telemetry#28663, especially in migration scenarios we want to receive both versions of the same histogram, the classic explicit and exponential bucket histogram. The current internal model of the receiver does not allow this, so I modified the model to not only key metrics by metric family name, but also whether it's an exponential histogram. Some callbacks from Promtheus scrape don't tell us what the metric type is up front. For AppendExemplar to work I made transaction stateful to remember if it appended an exponential histogram before. For Append of staleness marker to work, I modified the algorithm to discover exponential histogram from the cached metadata and the name. Due to prometheus/prometheus#16217 I also had to modify AppendExemplar to detect trying to add exemplars from native histograms even if native histograms are turned off. #### Link to tracking issue Related to open-telemetry#26555 #### Testing Unit test updated. #### Documentation N/A - does not contradict. --------- Signed-off-by: György Krajcsovits <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I'd like to fix a technical dept from #28663 where the Prometheus receiver didn't allow receiving both classic and native histogram for the same metric name. To test the implementation, the assertions must not assume that the metric name alone is identifying in OpenTelemetry.
No new tests added, I just refactored to make name+type+unit the identifier.
Also I made it implicit that
doComparefails on metrics that were not expected - excluding the 5 scrape metrics. This means thatassertMetricAbsentis not needed anymore.