Skip to content

feat(api-metrics): rename metric instruments to match feature-freeze API specification#2496

Merged
obecny merged 11 commits intoopen-telemetry:mainfrom
legendecas:metrics-ff/metrics-type
Oct 15, 2021
Merged

feat(api-metrics): rename metric instruments to match feature-freeze API specification#2496
obecny merged 11 commits intoopen-telemetry:mainfrom
legendecas:metrics-ff/metrics-type

Conversation

@legendecas
Copy link
Copy Markdown
Member

@legendecas legendecas commented Sep 24, 2021 •
edited
Loading

This PR applies following renames regarding to the feature-freeze metrics API spec:

  • s/ValueRecorder/Histogram/
  • s/ValueObserver/ObservableGauge/
  • s/SumObserver/ObservableCounter/
  • s/UpDownSumObserver/ObservableUpDownCounter/
  • s/BoundBaseObserver/BoundObservableBase/

There is no functional change.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 24, 2021 •
edited
Loading

Codecov Report

Merging #2496 (2ad4109) into main (96dab9f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2496   +/-   ##
=======================================
  Coverage   93.05%   93.05%           
=======================================
  Files         139      139           
  Lines        5172     5172           
  Branches     1110     1110           
=======================================
  Hits         4813     4813           
  Misses        359      359
Impacted Files Coverage Δ
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts 82.35% <0.00%> (ø)
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <0.00%> (ø)
...emetry-sdk-metrics-base/src/ValueRecorderMetric.ts
...lemetry-sdk-metrics-base/src/BaseObserverMetric.ts
...elemetry-sdk-metrics-base/src/SumObserverMetric.ts
...ry-sdk-metrics-base/src/UpDownSumObserverMetric.ts
...emetry-sdk-metrics-base/src/ValueObserverMetric.ts
...entelemetry-sdk-metrics-base/src/ObserverResult.ts
...ntelemetry-sdk-metrics-base/src/HistogramMetric.ts 100.00% <0.00%> (ø)
...-metrics-base/src/ObservableUpDownCounterMetric.ts 100.00% <0.00%> (ø)
... and 4 more

@legendecas legendecas marked this pull request as ready for review September 24, 2021 04:14
@legendecas legendecas requested a review from a team September 24, 2021 04:14
@legendecas legendecas force-pushed the metrics-ff/metrics-type branch from a8ae8cd to 53768bd Compare September 24, 2021 04:17
@legendecas legendecas changed the title feat: rename metric instruments to match feature-freeze API specification feat(api-metrics): rename metric instruments to match feature-freeze API specification Sep 24, 2021
vmarchaud
vmarchaud approved these changes Sep 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

pirgeo
pirgeo reviewed Oct 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@pirgeo pirgeo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The specification "highly recommends" ObservableCounter, ObservableGauge, and ObservableUpDownCounter. Is there a reason or strong opinion for the deviation here (CounterObserver, GaugeObserver, UpDownCounterObserver)?

@dyladan
Copy link
Copy Markdown
Member

dyladan commented Oct 4, 2021

The specification "highly recommends" ObservableCounter, ObservableGauge, and ObservableUpDownCounter. Is there a reason or strong opinion for the deviation here (CounterObserver, GaugeObserver, UpDownCounterObserver)?

I can only guess what @legendecas logic is here, but my guess is that it is to have the least disruption to what people already know. JS already had {}Observer naming so I think he just kept it.

I would prefer to go with the spec on this one though. We will be changing a lot of the metric spec anyway so there isn't a strong reason to keep what we have.

@pirgeo pirgeo mentioned this pull request Oct 5, 2021
Closed
14 tasks
@dyladan
Copy link
Copy Markdown
Member

dyladan commented Oct 5, 2021

@legendecas are you planning no making the suggested naming changes?

@legendecas
Copy link
Copy Markdown
Member Author

legendecas commented Oct 5, 2021

@dyladan If people are ok with the Observable* naming, I'm going to update the PR late today.

@legendecas
Merge branch 'main' into metrics-ff/metrics-type
b3348f1 
# Conflicts:
#	experimental/packages/opentelemetry-exporter-otlp-grpc/protos
#	experimental/packages/opentelemetry-exporter-otlp-proto/protos
#	experimental/tsconfig.esm.json
#	experimental/tsconfig.json
@legendecas
fixup! rename observables
8d88222
@pirgeo pirgeo mentioned this pull request Oct 6, 2021
Merged
pirgeo
pirgeo approved these changes Oct 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

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

Great Work! I just added a few nits, but all of these are personal preference, so please disregard as you see fit.

Comment thread experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts Outdated
@legendecas
Copy link
Copy Markdown
Member Author

legendecas commented Oct 7, 2021

@pirgeo thanks for the review, updated!

vmarchaud
vmarchaud approved these changes Oct 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm, i think we should wait for #2485 before merging this one though

pragmaticivan
pragmaticivan reviewed Oct 7, 2021
View reviewed changes
Comment thread experimental/packages/opentelemetry-api-metrics/src/types/Observation.ts Outdated
obecny
obecny reviewed Oct 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

Some minor things and few missing, but I also found some replacement which should be different, not sure if that is a mistake then or you rearranged the order but then maybe it will be easier to revert just the order so the replacing will be covered with those for easier review

Synchronous
Counter -> Counter
UpDownCounter -> UpDownCounter
ValueRecorder -> Histogram

Asynchronous
SumObserver -> ObservableCounter
UpDownSumObserver -> ObservableUpDownCounter
ValueObserver -> ObservableGauge

Please update also Upgrade guidelines listing what has changed and what not

Comment thread examples/metrics/metrics/observer.js Outdated
Comment thread examples/metrics/metrics/observer.js Outdated
Comment thread examples/metrics/metrics/observer.js Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Comment thread experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts Outdated
Comment thread experimental/packages/opentelemetry-api-metrics/src/types/ObserverResult.ts Outdated
Comment thread ...ental/packages/opentelemetry-exporter-otlp-http/test/browser/CollectorMetricExporter.test.ts Outdated
@legendecas
Copy link
Copy Markdown
Member Author

legendecas commented Oct 14, 2021

@obecny Reverted the reordering. Sorry for the inconvenience. Updated the PR with minor observable renaming.

@legendecas legendecas force-pushed the metrics-ff/metrics-type branch from 7187ad3 to 68f408e Compare October 14, 2021 02:57
obecny
obecny approved these changes Oct 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

looks great, thx for changes

@obecny obecny merged commit f9f43e5 into open-telemetry:main Oct 15, 2021
@legendecas legendecas deleted the metrics-ff/metrics-type branch October 17, 2021 03:30
willarmiros pushed a commit to willarmiros/opentelemetry-js that referenced this pull request Oct 26, 2021
applied changes from open-telemetry#2496 to metricHelpers
8b3571f
@dyladan dyladan mentioned this pull request Nov 10, 2021
Merged
@pirgeo pirgeo mentioned this pull request Nov 30, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dyladan dyladan Awaiting requested review from dyladan

6 more reviewers

@pragmaticivan pragmaticivan pragmaticivan left review comments

@weyert weyert weyert approved these changes

@obecny obecny obecny approved these changes

@rauno56 rauno56 rauno56 approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@pirgeo pirgeo pirgeo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@legendecas @dyladan @weyert @pragmaticivan @obecny @rauno56 @vmarchaud @pirgeo