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

[exporter/prometheus] Fix panic when mutating data #29608

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

crobert-1
Copy link
Member

@crobert-1 crobert-1 commented Dec 1, 2023

Description:

The prometheus exporter hit a panic when accumulating Delta metrics into Cumulative sums. This is because the exporter does not enable mutating data in its capability. This change enables the exporter to mutate data in a safe and supported way.

Link to tracking Issue:
Fixed #29574

Testing
There are existing tests that hit the logic that was panicking, but the metrics are set to StateMutable in testing (which is the only way they can be created and setup for testing). I believe that means that before this change the tests were invalid (didn't represent reality), but after this change they'll properly represent the exporter's functionality.

@bryan-aguilar
Copy link
Contributor

I believe that means that before this change the tests were invalid (didn't represent reality)

Is there any way to format/change the tests so that this isn't the case?

@crobert-1
Copy link
Member Author

crobert-1 commented Dec 1, 2023

I believe that means that before this change the tests were invalid (didn't represent reality)

Is there any way to format/change the tests so that this isn't the case?

I looked into it a bit, I couldn't find a way with existing tests. The way to test this is to create the exporter and be able to consume test data and ensure that the test data has its state set to Mutable. However, all test methods have to set the data to Mutable so they can actually create it.

I think the best way for this to work would be attempting to use the telemetrygen tool, if it can create metrics in a more realistic way, and then data can be passed through a collector pipeline with the Prometheus exporter. Then we could check metrics coming in to see if they're set to mutable or not.

I was able to test manually though and confirmed that metrics were ReadOnly before the change and Mutable after.

@atoulme
Copy link
Contributor

atoulme commented Dec 6, 2023

Please rebase

@codeboten codeboten merged commit 7d614d7 into open-telemetry:main Dec 8, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants