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(metrics): prototype experimental advice support #3876

Merged
merged 8 commits into from
Oct 11, 2023

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jun 8, 2023

Which problem is this PR solving?

Fixes #3722, #3619

Short description of the changes

Type of change

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

How Has This Been Tested?

  • createHistogram with advice and verify the exported boundaries.

Checklist:

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

@legendecas legendecas requested a review from a team June 8, 2023 10:03
@legendecas legendecas added api:metrics Issues and PRs related to the Metrics API sdk:metrics Issues and PRs related to the Metrics SDK labels Jun 8, 2023
@legendecas legendecas force-pushed the metrics/advice branch 3 times, most recently from 181232e to 8b2188e Compare June 8, 2023 11:05
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #3876 (1fff3c4) into main (f8e187b) will increase coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head 1fff3c4 differs from pull request most recent head 10966bb. Consider uploading reports for the commit 10966bb to get more accurate results

@@            Coverage Diff             @@
##             main    #3876      +/-   ##
==========================================
+ Coverage   92.24%   92.53%   +0.28%     
==========================================
  Files         331      317      -14     
  Lines        9479     8907     -572     
  Branches     1999     1877     -122     
==========================================
- Hits         8744     8242     -502     
+ Misses        735      665      -70     
Files Coverage Δ
api/src/metrics/Metric.ts 100.00% <ø> (ø)
packages/sdk-metrics/src/InstrumentDescriptor.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/ObservableResult.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/Drop.ts 100.00% <ø> (ø)
...sdk-metrics/src/aggregator/ExponentialHistogram.ts 98.12% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/Histogram.ts 92.00% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/LastValue.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/Sum.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/types.ts 100.00% <ø> (ø)
packages/sdk-metrics/src/export/MetricData.ts 100.00% <ø> (ø)
... and 3 more

... and 15 files with indirect coverage changes

@legendecas legendecas linked an issue Jun 8, 2023 that may be closed by this pull request
@legendecas
Copy link
Member Author

open-telemetry/opentelemetry-specification#3391 (comment) The advice API needs one more language implementation to be advanced to stable status.

@open-telemetry/javascript-approvers would you mind taking a look at this? Thank you!

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.

Seems fine to me. The only real functional change appears to be in Aggregation.ts. Everything else is names/types/organization

@@ -207,7 +211,7 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
}

toMetricData(
descriptor: InstrumentDescriptor,
descriptor: MetricDescriptor,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is needed to avoid exposing the advice on the InstrumentDescriptor to the MetricReaders and the MetricExporters.

# Conflicts:
#	CHANGELOG.md
#	packages/sdk-metrics/src/ObservableResult.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.

Thanks for working on this 🙂
Added a few comments around documenting stability/understanding precedence for advice in the SDK.

api/src/metrics/Metric.ts Show resolved Hide resolved
api/src/metrics/Metric.ts Show resolved Hide resolved
api/src/metrics/Metric.ts Show resolved Hide resolved
packages/sdk-metrics/src/export/MetricData.ts Show resolved Hide resolved
packages/sdk-metrics/src/view/Aggregation.ts Show resolved Hide resolved
legendecas and others added 5 commits October 7, 2023 17:11
# Conflicts:
#	CHANGELOG.md
#	api/CHANGELOG.md
#	packages/sdk-metrics/src/InstrumentDescriptor.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.

Thank you for working on this. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:metrics Issues and PRs related to the Metrics API sdk:metrics Issues and PRs related to the Metrics SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the histogram bucket metric advise API Easier way to specify histogram boundaries?
3 participants