Skip to content

[reciever/prometheusremotewritereceiver] add help ref #38764

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

Merged
merged 4 commits into from
Mar 18, 2025

Conversation

perebaj
Copy link
Contributor

@perebaj perebaj commented Mar 18, 2025

Description

This PR belongs to part of the Linux foundation mentee program.
Here we are adding the help ref attribute to metric following the rules described here

Link to tracking issue

Fixes part of #37277

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Any reason for the PR still being in draft mode? Anything you'd like to fix before marking as ready?

Comment on lines +324 to +327
// The only difference between ts 0 and 1 is the value assigned in the HelpRef. According to the spec
// Ref: https://opentelemetry.io/docs/specs/otel/metrics/data-model/#opentelemetry-protocol-data-model,
// the HelpRef(description) field is not considered an identifying property.
// This means that if you have two metrics with the same name, unit, scope, and resource attributes but different description values, they are still considered to be the same
Copy link
Member

Choose a reason for hiding this comment

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

really really nice find! thanks for paying attention to the spec!

@perebaj
Copy link
Contributor Author

perebaj commented Mar 18, 2025

Any reason for the PR still being in draft mode? Anything you'd like to fix before marking as ready?

Nope. I was just waiting for your approval to make sure that this make sense. Moving to ready mode ✌🏽

@perebaj perebaj marked this pull request as ready for review March 18, 2025 21:16
@perebaj perebaj requested a review from a team as a code owner March 18, 2025 21:16
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

What do you think about adding a test that asserts the out of bounds error for helpRef as well?

@perebaj
Copy link
Contributor Author

perebaj commented Mar 18, 2025

What do you think about adding a test that asserts the out of bounds error for helpRef as well?

Sorry, I completely forgot it. Adding...

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Thank you! You're on fire today :)

@ArthurSens ArthurSens added the ready to merge Code review completed; ready to merge by maintainers label Mar 18, 2025
@atoulme atoulme merged commit 67ee05a into open-telemetry:main Mar 18, 2025
170 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 18, 2025
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…#38764)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR belongs to part of the Linux foundation mentee program.
Here we are adding the help ref attribute to metric following the rules
described
[here](https://opentelemetry.io/docs/specs/otel/metrics/data-model/#opentelemetry-protocol-data-model-producer-recommendations)

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes part of
[open-telemetry#37277](open-telemetry#37277)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheusremotewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants