Skip to content

PeriodicExportingMetricReader will continue collection times out (#3098) #3100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 23, 2023
Merged

PeriodicExportingMetricReader will continue collection times out (#3098) #3100

merged 1 commit into from
Feb 23, 2023

Conversation

markallanson
Copy link
Contributor

@markallanson markallanson commented Dec 21, 2022 •
edited
Loading

Description

In cases where collection times out, the period exporting reader thread should not terminate, but instead catch, log, and continue on after the regular interval seconds.

Prior to this commit, a metric collection timeout would terminate the thread and stop reporting metrics to the wrapped exporter resulting in the appearance in observability tooling of metrics just stopping without reason.

Fixes #3098

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Added a new test to ensure the OtelPeriodicExportingMetricReader thread remains alive after a MetricsTimeoutError timeout error is raised by the collection process
  • Added a new test to ensure the OtelPeriodicExportingMetricReader thread is killed for other exception types

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No. This is an SDK bugfix only.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@markallanson markallanson requested a review from a team December 21, 2022 10:00
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 21, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@markallanson
Copy link
Contributor Author

Getting our company setup on EasyCLA at the moment.

@srikanthccv
Copy link
Member

@markallanson is there any chance you can get CLA signed this week? I would like to get this included in the upcoming monthly release.

@markallanson
Copy link
Contributor Author

@markallanson is there any chance you can get CLA signed this week? I would like to get this included in the upcoming monthly release.

I am off work this week but will follow up with legal/approver our side and see what can be done.

aabmass
aabmass approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@yulin-li
Copy link

yulin-li commented Jan 17, 2023 •
edited
Loading

I am facing this issue too, could we get this PR merged? Thanks

@markallanson
Copy link
Contributor Author

I am still chasing for the corporate CLA to be signed from our side. Will merge this in as soon as I can.

@markallanson
Copy link
Contributor Author

markallanson commented Feb 22, 2023 •
edited
Loading

@srikanthccv The company is now configured for EasyCLA and all signed so can go ahead and merge this change.

@srikanthccv
Copy link
Member

Please add a CHANGELOG entry and fix any workflow failures (there were some failures last time).

@markallanson markallanson force-pushed the handle_metric_timeout_periodic_exporter branch 2 times, most recently from 0b3f66c to cd522e7 Compare February 23, 2023 10:44
srikanthccv
srikanthccv reviewed Feb 23, 2023
View reviewed changes
@markallanson markallanson force-pushed the handle_metric_timeout_periodic_exporter branch 2 times, most recently from d185889 to 277ccad Compare February 23, 2023 17:51
@markallanson
PeriodicExportingMetricReader will continue collection times out (ope…
4e1307f 
…n-telemetry#3098)

In cases where collection times out, the period exporting reader thread should
not terminate, but instead catch, log, and continue on after the regular interval
seconds.

Prior to this commit, a metric collection timeout would terminate the thread and
stop reporting metrics to the wrapped exporter resulting in the appearance in
observability tooling of metrics just stopping without reason.
@markallanson markallanson force-pushed the handle_metric_timeout_periodic_exporter branch from 277ccad to 4e1307f Compare February 23, 2023 17:51
@srikanthccv srikanthccv merged commit 559e06d into open-telemetry:main Feb 23, 2023
@dcdennis dcdennis mentioned this pull request Oct 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@ccorbacho ccorbacho ccorbacho approved these changes

@lzchen lzchen lzchen approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PeriodicExportingMetricReader thread dies when metrics collection times out
6 participants
@markallanson @srikanthccv @yulin-li @aabmass @ccorbacho @lzchen