-
Notifications
You must be signed in to change notification settings - Fork 804
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(api-metrics): rename metric instruments to match feature-freeze API specification #2496
feat(api-metrics): rename metric instruments to match feature-freeze API specification #2496
Conversation
a8ae8cd
to
53768bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
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
)?
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 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. |
@legendecas are you planning no making the suggested naming changes? |
@dyladan If people are ok with the |
# Conflicts: # experimental/packages/opentelemetry-exporter-otlp-grpc/protos # experimental/packages/opentelemetry-exporter-otlp-proto/protos # experimental/tsconfig.esm.json # experimental/tsconfig.json
There was a problem hiding this 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.
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
@pirgeo thanks for the review, updated! |
There was a problem hiding this 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
experimental/packages/opentelemetry-api-metrics/src/types/Observation.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-api-metrics/src/types/ObserverResult.ts
Outdated
Show resolved
Hide resolved
...ental/packages/opentelemetry-exporter-otlp-http/test/browser/CollectorMetricExporter.test.ts
Outdated
Show resolved
Hide resolved
@obecny Reverted the reordering. Sorry for the inconvenience. Updated the PR with minor observable renaming. |
7187ad3
to
68f408e
Compare
There was a problem hiding this 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
This PR applies following renames regarding to the feature-freeze metrics API spec:
There is no functional change.