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/awsemf] Improve summary metric type NaN checks #28894

Merged

Conversation

bryan-aguilar
Copy link
Contributor

Description: I have observed some behavior on a personal collector deployment where the EMF Exporter is still returning errors for NaN json marshalling. This was in a prometheus -> emf exporter metrics pipeline.

I could not find the specific NaN value in the metrics when troubleshooting the error. I curled the /metrics endpoint and also tried using the logging exporter to try to get more information. I could not find where the NaN value was coming from so I took another look into the unit tests and found some possible code paths in which NaNs could slip though.

Link to tracking Issue: Original issue #26267

Testing: Added more unit tests. The summary unit tests got a slight refactor for two reasons. So I could get ride of the unnecessary typecasting and so that we could more easily test out different combinations of quantile values.

I have also added a few more histogram unit tests to just verify that all combinations of NaN values are being checked on their own.

@bryan-aguilar bryan-aguilar requested a review from a team November 5, 2023 23:18
@github-actions github-actions bot added the exporter/awsemf awsemf exporter label Nov 5, 2023
@bryan-aguilar bryan-aguilar changed the title [exporter/awsemf] Improve summary NaN checks [exporter/awsemf] Improve summary metric type NaN checks Nov 5, 2023
@andrzej-stencel
Copy link
Member

Can a codeowner take a look? @Aneurysm9 @shaochengwang @mxiamxia

@Aneurysm9 Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Nov 15, 2023
@TylerHelmuth TylerHelmuth merged commit 759ee9e into open-telemetry:main Nov 16, 2023
88 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 16, 2023
graphaelli pushed a commit to graphaelli/opentelemetry-collector-contrib that referenced this pull request Nov 17, 2023
…try#28894)

**Description:** I have observed some behavior on a personal collector
deployment where the EMF Exporter is still returning errors for `NaN`
json marshalling. This was in a prometheus -> emf exporter metrics
pipeline.

I could not find the specific NaN value in the metrics when
troubleshooting the error. I curled the `/metrics` endpoint and also
tried using the logging exporter to try to get more information. I could
not find where the NaN value was coming from so I took another look into
the unit tests and found some possible code paths in which NaNs could
slip though.

**Link to tracking Issue:** Original issue
open-telemetry#26267

**Testing:** Added more unit tests. The summary unit tests got a slight
refactor for two reasons. So I could get ride of the unnecessary
typecasting and so that we could more easily test out different
combinations of quantile values.

I have also added a few more histogram unit tests to just verify that
all combinations of NaN values are being checked on their own.
@bryan-aguilar bryan-aguilar deleted the awsemf/SummaryNanCheck branch November 17, 2023 21:06
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
…try#28894)

**Description:** I have observed some behavior on a personal collector
deployment where the EMF Exporter is still returning errors for `NaN`
json marshalling. This was in a prometheus -> emf exporter metrics
pipeline.

I could not find the specific NaN value in the metrics when
troubleshooting the error. I curled the `/metrics` endpoint and also
tried using the logging exporter to try to get more information. I could
not find where the NaN value was coming from so I took another look into
the unit tests and found some possible code paths in which NaNs could
slip though.

**Link to tracking Issue:** Original issue
open-telemetry#26267

**Testing:** Added more unit tests. The summary unit tests got a slight
refactor for two reasons. So I could get ride of the unnecessary
typecasting and so that we could more easily test out different
combinations of quantile values.

I have also added a few more histogram unit tests to just verify that
all combinations of NaN values are being checked on their own.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/awsemf awsemf exporter ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants