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.

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

feat(sdk-metrics-base): update exporting names #2829

Merged
merged 5 commits into from
Mar 21, 2022
Merged

feat(sdk-metrics-base): update exporting names #2829

merged 5 commits into from
Mar 21, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 14, 2022 •
edited
Loading

Which problem is this PR solving?

  1. Exported Histogram Instrument class name is conflict with the data type Histogram. Renames all instrument classes with the suffix Instrument.
  2. Proper field names in MetricData: plural pointData -> dataPoints, shorten instrumentDescriptor -> descriptor since the interface already represents an instrument.
  3. HistogramAggregation accepts a parameter to explicitly set the boundaries, as the spec defined.

Extracted from #2824

Type of change

  • Breaking change, but the package is not released yet.

Checklist:

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

@codecov
Copy link

codecov bot commented Mar 14, 2022 •
edited
Loading

Codecov Report

Merging #2829 (3688371) into main (70dc60b) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2829   +/-   ##
=======================================
  Coverage   93.51%   93.52%           
=======================================
  Files         163      163           
  Lines        5570     5572    +2     
  Branches     1179     1180    +1     
=======================================
+ Hits         5209     5211    +2     
  Misses        361      361
Impacted Files Coverage Δ
...telemetry-sdk-metrics-base/src/view/Aggregation.ts 100.00% <0.00%> (ø)

@legendecas legendecas marked this pull request as ready for review March 14, 2022 07:14
@legendecas legendecas requested a review from a team March 14, 2022 07:14
@legendecas legendecas mentioned this pull request Mar 15, 2022
Merged
2 tasks
@legendecas legendecas requested a review from a team March 17, 2022 15:14
@aabmass aabmass mentioned this pull request Mar 18, 2022
Closed
@legendecas
Merge remote-tracking branch 'upstream/main' into metrics-ff/exporters
9fcf82e 
# Conflicts:
#	experimental/packages/opentelemetry-sdk-metrics-base/src/index.ts
#	experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts
@legendecas legendecas force-pushed the metrics-ff/exporters branch from 96ef778 to 9fcf82e Compare March 21, 2022 02:57
pichlermarc
pichlermarc approved these changes Mar 21, 2022
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.

LGTM! 🙂

Just one nit: the rename from _instrumentDescriptor -> _descriptor is still missing in aggregator/Drop.ts

@vmarchaud vmarchaud merged commit 3fd1b1e into open-telemetry:main Mar 21, 2022
@legendecas legendecas deleted the metrics-ff/exporters branch March 21, 2022 14:16
@legendecas legendecas mentioned this pull request Mar 28, 2022
Merged
6 tasks
@pichlermarc pichlermarc mentioned this pull request Apr 22, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc approved these changes

@seemk seemk seemk approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@rauno56 rauno56 rauno56 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.
5 participants
@legendecas @rauno56 @vmarchaud @seemk @pichlermarc