Skip to content

[receiver/prometheusremotewritereceiver] accept unspecified histogram #43901

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[receiver/prometheusremotewritereceiver] accept unspecified histogram #43901

wants to merge 3 commits into from
+225 −8

Conversation

@jelly-afk
Copy link
Contributor

@jelly-afk jelly-afk commented Oct 30, 2025

Description

prometheusremote write receiver now accepts histogram with unspecified metric type.

Link to tracking issue

Fixes #41840

Testing

Added a testcase in TestTranslateV2 test function to test unspecified histogram.

@jelly-afk jelly-afk marked this pull request as ready for review October 30, 2025 13:20
@jelly-afk jelly-afk requested review from a team, ArthurSens and dashpole as code owners October 30, 2025 13:20
@github-actions github-actions bot requested a review from perebaj October 30, 2025 13:20
dashpole
dashpole reviewed Oct 30, 2025
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for CBNH, not standard "split" histograms (e.g. with _bucket _sum and _count series). Will this actually fix your use-case? I'm very surprised that you can get a CBNH without a histogram type...

@ArthurSens
Copy link
Member

ArthurSens commented Nov 4, 2025

This will only work for CBNH, not standard "split" histograms (e.g. with _bucket _sum and _count series). Will this actually fix your use-case? I'm very surprised that you can get a CBNH without a histogram type...

I think that's the common case when Prometheus doesn't have metadata-wal-records enabled, isn't it? Prometheus remote-write heavily depends on WAL, so if metadata is not present in WAL, then everything is sent without type.

ArthurSens
ArthurSens reviewed Nov 4, 2025
View reviewed changes
receiver/prometheusremotewritereceiver/receiver_test.go
}(),
},
{
name: "unspecified histogram",
Copy link
Member

@ArthurSens ArthurSens Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add one extra test that has Native histograms with custom buckets? Those are the histograms with schema == -53, you'll probably find examples in other tests in this same file

perebaj
perebaj reviewed Nov 5, 2025
View reviewed changes
receiver/prometheusremotewritereceiver/receiver.go Outdated
Comment on lines 491 to 495
if ts.Metadata.Type == writev2.Metadata_METRIC_TYPE_HISTOGRAM {
histMetric.Metadata().PutStr(prometheus.MetricMetadataTypeKey, "histogram")
} else {
histMetric.Metadata().PutStr(prometheus.MetricMetadataTypeKey, "unknown")
}
Copy link
Contributor

@perebaj perebaj Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't better to use switch case for this use case?

receiver/prometheusremotewritereceiver/receiver.go
Comment on lines +304 to +345
if ts.Metadata.Type == writev2.Metadata_METRIC_TYPE_HISTOGRAM ||
ts.Metadata.Type == writev2.Metadata_METRIC_TYPE_UNSPECIFIED && len(ts.Histograms) > 0 {
Copy link
Contributor

@perebaj perebaj Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we receive a Metadata_METRIC_TYPE_UNSPECIFIED with other type of time series, different than Histograms? Or this is an impossible scenario?

Copy link
Contributor Author

@jelly-afk jelly-afk Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean other metric types besides histograms? 🤔
If so, those were handled in #42295

Copy link
Contributor

@perebaj perebaj Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. thanks!

@perebaj
Copy link
Contributor

perebaj commented Nov 5, 2025

Thanks for that @jelly-afk. Just 2 nits.

@jelly-afk
Copy link
Contributor Author

jelly-afk commented Nov 6, 2025

This will only work for CBNH, not standard "split" histograms (e.g. with _bucket _sum and _count series). Will this actually fix your use-case? I'm very surprised that you can get a CBNH without a histogram type...

Hi, I don't have similar use case. I opened this because I worked on the first part of the issue.
As for standard histograms i can try looking into those as well.

jelly-afk and others added 3 commits November 6, 2025 23:07
@jelly-afk
accept unspecified histogram
406309f
@perebaj @VihasMakwana
@ArthurSens @jelly-afk
[prometheusremotewritereceiver] concurrency bug (open-telemetry#43383)
37767c6 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

Fixed a concurrency bug in the Prometheus remote write receiver where
concurrent requests with identical job/instance labels would return
empty responses after the first successful request.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
Fixes open-telemetry#42159

<!--Describe what testing was performed and which tests were added.-->

<!--Describe the documentation added.-->

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: perebaj <[email protected]>
Co-authored-by: Vihas Makwana <[email protected]>
Co-authored-by: Arthur Silva Sens <[email protected]>
@jelly-afk
add unspecified nhcb test case
53bed3e
@jelly-afk jelly-afk force-pushed the accept-unspecified-histogram branch from afe1b57 to 53bed3e Compare November 6, 2025 17:38
@jelly-afk
Copy link
Contributor Author

jelly-afk commented Nov 6, 2025

sorry, i will open a new one

@jelly-afk jelly-afk closed this Nov 6, 2025
@jelly-afk jelly-afk deleted the accept-unspecified-histogram branch November 7, 2025 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dashpole dashpole dashpole left review comments

@ArthurSens ArthurSens ArthurSens left review comments

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel

@crobert-1 crobert-1 Awaiting requested review from crobert-1

@atoulme atoulme Awaiting requested review from atoulme

@pjanotti pjanotti Awaiting requested review from pjanotti

@VihasMakwana VihasMakwana Awaiting requested review from VihasMakwana

@dmitryax dmitryax Awaiting requested review from dmitryax

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146

@dehaansa dehaansa Awaiting requested review from dehaansa

@ChrsMark ChrsMark Awaiting requested review from ChrsMark

@evan-bradley evan-bradley Awaiting requested review from evan-bradley

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth

@axw axw Awaiting requested review from axw

@mwear mwear Awaiting requested review from mwear

@codeboten codeboten Awaiting requested review from codeboten

@braydonk braydonk Awaiting requested review from braydonk

@mx-psi mx-psi Awaiting requested review from mx-psi

@edmocosta edmocosta Awaiting requested review from edmocosta

@songy23 songy23 Awaiting requested review from songy23

@jsuereth jsuereth Awaiting requested review from jsuereth

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@aabmass aabmass Awaiting requested review from aabmass

@bogdan-st bogdan-st Awaiting requested review from bogdan-st

@Erog38 Erog38 Awaiting requested review from Erog38

@kuiperda kuiperda Awaiting requested review from kuiperda

@psx95 psx95 Awaiting requested review from psx95

@ridwanmsharif ridwanmsharif Awaiting requested review from ridwanmsharif

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

+1 more reviewer

@perebaj perebaj perebaj left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@TylerHelmuth TylerHelmuth

Labels

cmd/golden cmd/opampsupervisor cmd/otelcontribcol otelcontribcol command cmd/oteltestbedcol cmd/telemetrygen telemetrygen command confmap/provider/aesprovider confmap/provider/googlesecretmanagerprovider receiver/prometheusremotewrite

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[receiver/prometheusremotewrite] Support METRIC_TYPE_UNSPECIFIED

5 participants

@jelly-afk @ArthurSens @perebaj @dashpole @TylerHelmuth