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

feat(sdk-metrics-base): async instruments callback timeout #2742

Merged
merged 9 commits into from
May 12, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 27, 2022

Which problem is this PR solving?

Apply timeout option directly to each async instrument callback to gain finer grain control over timeout. With open-telemetry/opentelemetry-specification#2295 being clarified, we can export successfully collected metric data even if a single async instrument callback is timed out.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • callWithTimeout
  • MetricCollector.collect

Checklist:

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

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #2742 (4842a06) into main (65fbb2f) will increase coverage by 0.00%.
The diff coverage is 95.55%.

@@           Coverage Diff           @@
##             main    #2742   +/-   ##
=======================================
  Coverage   92.51%   92.52%           
=======================================
  Files         183      183           
  Lines        5959     5980   +21     
  Branches     1266     1268    +2     
=======================================
+ Hits         5513     5533   +20     
- Misses        446      447    +1     
Impacted Files Coverage Δ
...elemetry-sdk-metrics-base/src/export/MetricData.ts 100.00% <ø> (ø)
...s-base/src/export/PeriodicExportingMetricReader.ts 91.11% <75.00%> (ø)
...etry-exporter-prometheus/src/PrometheusExporter.ts 92.40% <80.00%> (-1.19%) ⬇️
...emetry-sdk-metrics-base/src/export/MetricReader.ts 100.00% <100.00%> (ø)
...try-sdk-metrics-base/src/state/MeterSharedState.ts 97.50% <100.00%> (ø)
...etry-sdk-metrics-base/src/state/MetricCollector.ts 100.00% <100.00%> (ø)
...y-sdk-metrics-base/src/state/ObservableRegistry.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts 100.00% <100.00%> (ø)

@legendecas legendecas force-pushed the metrics-ff/async-timeout branch from 240e96c to 011e528 Compare January 27, 2022 07:44
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the stale label Apr 11, 2022
@dyladan dyladan removed the stale label Apr 19, 2022
@dyladan
Copy link
Member

dyladan commented Apr 19, 2022

This PR is not stale it is just still waiting on a spec clarification which has not yet been closed.

@legendecas
Copy link
Member Author

The spec change has been merged: open-telemetry/opentelemetry-specification#2495. I'm going to pick this up.

@legendecas

This comment was marked as outdated.

@legendecas legendecas force-pushed the metrics-ff/async-timeout branch 2 times, most recently from 3e7b382 to 56cb7a5 Compare May 9, 2022 07:56
@legendecas legendecas force-pushed the metrics-ff/async-timeout branch from 56cb7a5 to 05b0e6e Compare May 9, 2022 08:26
@legendecas legendecas marked this pull request as ready for review May 9, 2022 08:40
@legendecas legendecas requested review from a team and pichlermarc May 9, 2022 08:40
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM % small comments

@legendecas legendecas requested a review from a team May 11, 2022 02:28
…meout

# Conflicts:
#	experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterSharedState.ts
#	experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts
#	experimental/packages/opentelemetry-sdk-metrics-base/test/export/PeriodicExportingMetricReader.test.ts
#	experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts
…meout-stage

# Conflicts:
#	experimental/CHANGELOG.md
#	experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the great work! 🙂

@legendecas legendecas merged commit aabc5f6 into open-telemetry:main May 12, 2022
@legendecas legendecas deleted the metrics-ff/async-timeout branch May 12, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants