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

chore: fix handling of metrics with created suffix #30808

Conversation

jmichalek132
Copy link
Contributor

Description:
As discussed in the OTEL Prometheus working group, to fix this case we check If the metricfamily (which we get from the metadata) name + _created == the series name match. Then we treat it as created timestamp.
Link to tracking Issue: #30309

Testing: Reproduced based on the details provided in the issue, added a test case covering the case.

Documentation:

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.

do we have tests that check that _created series are correctly parsed for summaries, histograms, and counters?

@jmichalek132
Copy link
Contributor Author

do we have tests that check that _created series are correctly parsed for summaries, histograms, and counters?

I added a test case for counters, will add one for summaries and histograms too.

@jmichalek132
Copy link
Contributor Author

do we have tests that check that _created series are correctly parsed for summaries, histograms, and counters?

Okay, so double checked there are positive tests for _created when it's actually _created timestamp being parsed and used for all of the three. We have also a test case for a counter where _created is not the actual created timestamp and is being left as is. Should I add a test case for the latter for summaries and histograms too?

@fatsheep9146
Copy link
Contributor

do we have tests that check that _created series are correctly parsed for summaries, histograms, and counters?

Okay, so double checked there are positive tests for _created when it's actually _created timestamp being parsed and used for all of the three. We have also a test case for a counter where _created is not the actual created timestamp and is being left as is. Should I add a test case for the latter for summaries and histograms too?

I think we'd better cover them all.

@dashpole
Copy link
Contributor

Discussed at the prometheus wg meeting. There are already tests for the _created suffix that work correctly.

@jmichalek132
Copy link
Contributor Author

Pipeline failure for a different component unrelated to my changes.

@dashpole dashpole added bug Something isn't working ready to merge Code review completed; ready to merge by maintainers labels Feb 5, 2024
@codeboten codeboten merged commit 81cfc48 into open-telemetry:main Feb 13, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 13, 2024
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
)

Description:
As discussed in the OTEL Prometheus working group, to fix this case we
check If the metricfamily (which we get from the metadata) name +
_created == the series name match. Then we treat it as created
timestamp.
Link to tracking Issue:
open-telemetry#30309

Testing: Reproduced based on the details provided in the issue, added a
test case covering the case.

Documentation:

---------

Co-authored-by: David Ashpole <[email protected]>
@jmichalek132 jmichalek132 deleted the jm-fix-handling-of-metrics-with-created-suffix-2 branch March 15, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready to merge Code review completed; ready to merge by maintainers receiver/prometheus Prometheus receiver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants