Skip to content

feat(metrics): prototype experimental advice support#3876

Merged
pichlermarc merged 8 commits intoopen-telemetry:mainfrom
legendecas:metrics/advice
Oct 11, 2023
Merged

feat(metrics): prototype experimental advice support#3876
pichlermarc merged 8 commits intoopen-telemetry:mainfrom
legendecas:metrics/advice

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Jun 8, 2023 •
edited
Loading

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 •
edited
Loading

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
Easier way to specify histogram boundaries? #3619
Closed
@legendecas legendecas mentioned this pull request Jul 7, 2023
Merged
5 tasks
@legendecas legendecas mentioned this pull request Jul 25, 2023
Closed
@legendecas
Copy link
Member Author

legendecas commented Jul 25, 2023

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!

dyladan
dyladan approved these changes Aug 11, 2023
View reviewed changes
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

packages/sdk-metrics/src/aggregator/Histogram.ts

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

@dyladan dyladan Aug 11, 2023

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

@legendecas legendecas Aug 16, 2023

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.

@legendecas
Merge branch 'main' into metrics/advice
8b30967 
# Conflicts:
#	CHANGELOG.md
#	packages/sdk-metrics/src/ObservableResult.ts
pichlermarc
pichlermarc reviewed Sep 13, 2023
View reviewed changes
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.

@pichlermarc pichlermarc mentioned this pull request Sep 18, 2023
Closed
6 tasks
@trask trask mentioned this pull request Sep 18, 2023
Merged
legendecas and others added 5 commits October 7, 2023 17:11
@legendecas
Merge branch 'main' into metrics/advice
dd3d00c 
# Conflicts:
#	CHANGELOG.md
#	api/CHANGELOG.md
#	packages/sdk-metrics/src/InstrumentDescriptor.ts
@legendecas
fixup!
37cdef3
@legendecas
fixup!
80eb992
@legendecas
fixup!
f4d12de
@pichlermarc
Merge branch 'main' into metrics/advice
10966bb
pichlermarc
pichlermarc approved these changes Oct 10, 2023
View reviewed changes
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. 🙂

@pichlermarc pichlermarc merged commit b6e532b into open-telemetry:main Oct 11, 2023
@legendecas legendecas deleted the metrics/advice branch October 11, 2023 09:55
@pichlermarc pichlermarc mentioned this pull request Nov 6, 2023
Merged
@kaylareopelle kaylareopelle mentioned this pull request Aug 16, 2024
Open
@lsfischer lsfischer mentioned this pull request Aug 21, 2024
Closed
@mhofman mhofman mentioned this pull request Jan 28, 2025
Merged
@benjdlambert benjdlambert mentioned this pull request May 13, 2025
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dyladan dyladan dyladan approved these changes

@pichlermarc pichlermarc pichlermarc approved these changes

Assignees

No one assigned

Labels

api:metrics Issues and PRs related to the Metrics API sdk:metrics Issues and PRs related to the Metrics SDK

Projects

None yet

Milestone

No milestone

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

@legendecas @dyladan @pichlermarc