[receiver/prometheusremotewritereceiver] accept unspecified histograms#44091
Conversation
jelly-afk
requested review from
a team,
ArthurSens and
dashpole
as code owners
| switch ts.Metadata.Type { | ||
| case writev2.Metadata_METRIC_TYPE_HISTOGRAM: | ||
| histMetric.Metadata().PutStr(prometheus.MetricMetadataTypeKey, "histogram") | ||
| default: |
There was a problem hiding this comment.
isn't not missing this case -> writev2.Metadata_METRIC_TYPE_UNSPECIFIED
jelly-afk
force-pushed
the
support-unspecified-histogram
branch
from
c6d7120 to
27a2adf
Compare
|
@jelly-afk Could you please fix the CI? |
jelly-afk
force-pushed
the
support-unspecified-histogram
branch
from
27a2adf to
0028929
Compare
VihasMakwana
left a comment
VihasMakwana
left a comment
There was a problem hiding this comment.
Looks good. Needs a codeowner review to be ready-to-merge
perebaj
left a comment
perebaj
left a comment
There was a problem hiding this comment.
Just some nits. But many thanks for that effort!
| default: | ||
| } |
There was a problem hiding this comment.
Maybe the default case should return an error? This because we are talking about a not listed case/
There was a problem hiding this comment.
@ArthurSens or @dashpole do you have a different opinion about it?
There was a problem hiding this comment.
We shouldn't encounter any other metric types here, as we have already validated the types here
if ts.Metadata.Type == writev2.Metadata_METRIC_TYPE_HISTOGRAM || ts.Metadata.Type == writev2.Metadata_METRIC_TYPE_UNSPECIFIED && len(ts.Histograms) > 0
So it can only be either a histogram or an unspecified histogram.
There was a problem hiding this comment.
What do you think if we add a comment under the default option with this info? Saying that as we are only dealing with histograms inside the processHistogramTimeSeries we don't need to define a behavior for this option.
| }, | ||
| Timeseries: []writev2.TimeSeries{ | ||
| { | ||
| LabelsRefs: []uint32{1, 2, 3, 4, 5, 6}, // __name__=test_hncb_histogram, job=test, instance=localhost:8080 |
There was a problem hiding this comment.
I think that this comment doesn't help us a lot. WDYT?
| LabelsRefs: []uint32{1, 2, 3, 4, 5, 6}, // __name__=test_hncb_histogram, job=test, instance=localhost:8080 | |
| LabelsRefs: []uint32{1, 2, 3, 4, 5, 6}, | |
| instance=localhost:8080 |
| "", | ||
| "__name__", | ||
| "test_hncb_histogram", | ||
| "job", | ||
| "test", | ||
| "instance", | ||
| "localhost:8080", | ||
| "seconds", | ||
| "Test NHCB histogram", |
There was a problem hiding this comment.
Can we follow the same pattern as the other test cases?
| "", | |
| "__name__", | |
| "test_hncb_histogram", | |
| "job", | |
| "test", | |
| "instance", | |
| "localhost:8080", | |
| "seconds", | |
| "Test NHCB histogram", | |
| "", | |
| "__name__", "test_hncb_histogram", // 1, 2 | |
| "job", "test", // 3, 4 | |
| "instance", "localhost:8080", // 5, 6 | |
| "seconds", "Test NHCB histogram", // 7, 8 |
Maybe accepting this change, the linter will break.
jelly-afk
force-pushed
the
support-unspecified-histogram
branch
from
a024087 to
f43b1bf
Compare
ArthurSens
added
the
ready to merge
|
Thank you for your contribution @jelly-afk! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
open-telemetry#44091) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description prometheusremote write receiver now accepts histogram with unspecified metric type. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#41840 <!--Describe what testing was performed and which tests were added.--> #### Testing Added two test cases in the TestTranslateV2 test function to test unspecified histograms. --------- Co-authored-by: Yang Song <songy23@users.noreply.github.com>
6 participants
Description
prometheusremote write receiver now accepts histogram with unspecified metric type.
Link to tracking issue
Fixes #41840
Testing
Added two test cases in the TestTranslateV2 test function to test unspecified histograms.