Skip to content
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

[prometheusreceiver] Fix metrics being grouped into the same metrics family incorrectly #13213

Merged
merged 10 commits into from
Oct 17, 2022

Conversation

newly12
Copy link
Contributor

@newly12 newly12 commented Aug 11, 2022

Description:
Fix metrics being grouped into the same metrics family incorrectly

Link to tracking Issue:
Fixes #13117

Testing:
UT

Documentation:
n/a

@newly12 newly12 requested a review from a team August 11, 2022 05:27
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.

Overall, I think this is a less bad thing to do.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 27, 2022
@newly12
Copy link
Contributor Author

newly12 commented Aug 29, 2022

@Aneurysm9 could you please comment?

@dashpole dashpole added receiver/prometheus Prometheus receiver and removed Stale labels Aug 29, 2022
@newly12 newly12 changed the title Fix metrics being grouped into the same metrics family incorrectly [prometheusreceiver] Fix metrics being grouped into the same metrics family incorrectly Aug 31, 2022
# Conflicts:
#	receiver/prometheusreceiver/internal/otlp_metricsbuilder.go
# Conflicts:
#	receiver/prometheusreceiver/internal/metricsbuilder.go
@bogdandrutu
Copy link
Member

@dashpole please do a final review and add the ready to merge label after.

@dashpole dashpole self-requested a review September 29, 2022 14:06
@dashpole
Copy link
Contributor

I think we should wait on this. I was able to discuss this at the prom wg yesterday (notes), and we concluded that we should see if it is possible for the appender to know if the metrics came from OpenMetrics or from Prometheus

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I think we should wait on this. I was able to discuss this at the prom wg yesterday (#13117 (comment)), and we concluded that we should see if it is possible for the appender to know if the metrics came from OpenMetrics or from Prometheus

@dashpole do you want to add a request for change until this is unblocked?

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.

lets see if we can get a feature in upstream promethetheus that lets us know if the scrape was openmetrics or prometheus

@newly12
Copy link
Contributor Author

newly12 commented Oct 13, 2022

Thanks @dashpole , do we have any prometheus issue being tracked for this currently?

@dashpole
Copy link
Contributor

While writing up the issue, i've realized that knowing openmetrics vs prometheus won't help us. I was going to write:

The OpenTelemetry collector's prometheus receiver is implemented as an Appender. One issue we currently have is that we don't know how to correctly associate metric points with type (or other) comments. For example, with the Prometheus metrics:

# TYPE foo gauge
foo{} 1
# TYPE foo_total counter
foo_total{} 1

Each metric point should be associated with the metric family of the same name (foo with foo, foo_total with foo_total)

However, with the OpenMetrics metrics:

# TYPE foo counter
foo_total{} 1
# TYPE foo_total gauge
foo_total{} 1

We want to associate the metrics with the counter, rather than the gauge.

But then I read in the OpenMetrics spec:

The name of a MetricFamily MUST NOT result in a potential clash for sample metric names as per the ABNF with another MetricFamily in the Text Format within a MetricSet. An example would be a gauge called "foo_created" as a counter called "foo" could create a "foo_created" in the text format.

So the OpenMetrics example above is invalid. In that case, I think it is correct for us to accept this PR.

# Conflicts:
#	receiver/prometheusreceiver/internal/transaction.go
@newly12
Copy link
Contributor Author

newly12 commented Oct 17, 2022

@codeboten @Aneurysm9 could you please review this PR?

@codeboten codeboten merged commit 06baf0c into open-telemetry:main Oct 17, 2022
@newly12 newly12 deleted the metricname_agg_issue branch October 18, 2022 05:01
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
…family incorrectly (open-telemetry#13213)

Fix metrics being grouped into the same metrics family incorrectly

Co-authored-by: David Ashpole <[email protected]>
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/prometheus Prometheus receiver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prometheusreceiver: possible incorrect metric name for cumulative metrics
5 participants