feat(api-metrics): async instruments spec compliance #2569

legendecas · 2021-10-27T17:19:35Z

Which problem is this PR solving?

Fixes #2550
Fixes #2551
Fixes #2553

Depends on #2566

Short description of the changes

Removes ObservableBase.observation
Removes type Observation from api-metrics
Swap callback(3rd parameter) with options(2nd parameteronMeter.createObservable*to markcallback` as a required parameter: a required parameter can not be placed after an optional parameter.
Mark the second parameter label of ObservableResult.observe as optional.

Type of change

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

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

codecov · 2021-10-27T17:22:17Z

Codecov Report

Merging #2569 (d8a666b) into main (9b5feb2) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2569      +/-   ##
==========================================
+ Coverage   93.08%   93.10%   +0.01%     
==========================================
  Files         123      123              
  Lines        4761     4758       -3     
  Branches     1061     1061              
==========================================
- Hits         4432     4430       -2     
+ Misses        329      328       -1

Impacted Files	Coverage Δ
...ages/opentelemetry-api-metrics/src/types/Metric.ts	`100.00% <0.00%> (ø)`
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts	`80.55% <0.00%> (+1.06%)`	⬆️

# Conflicts: # experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts # experimental/packages/opentelemetry-api-metrics/src/types/BatchObserverResult.ts # experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts # experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts

dyladan

Have you verified that observables are still useable after this change? By that I mean since you have removed observation does it still export metrics properly.

dyladan · 2021-11-03T15:18:53Z

experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts

-    labels?: Labels,
-  ) => Observation;
-}
+export type ObservableBase = Record<never, never>;


Record<never, never> only allows the empty object correct?

It declares an object type, and it can be extended to have any keys. The reason I use Record here is that the linter rules disallow empty interface interface ObservableBase {}.

dyladan · 2021-11-03T15:22:16Z

experimental/packages/opentelemetry-api-metrics/src/NoopMeter.ts

-    };
-  }
-}
+export class NoopObservableBaseMetric extends NoopMetric implements ObservableBase {}


Does NoopMetric serve any purpose here or anywhere?

Hm, the only use case can be observable instanceof NoopMetric. However, there are no API spec constraints on class hierarchy here, maybe we can remove it.

legendecas · 2021-11-03T15:47:51Z

@dyladan observation is only being used in batch observers. The observables can work with only ObservableResult:

const observable = meter.createObservableGauge('name', observableResult => {
  observableResult.observe(100, { attr1: 'val' });
});

// we don't have to call `observable.observation(100)` anymore since there is no BatchObserverResult to consume them.

# Conflicts: # experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts # experimental/packages/opentelemetry-api-metrics/src/types/ObservableResult.ts # experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts # experimental/packages/opentelemetry-sdk-metrics-base/README.md

# Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/README.md # experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts # experimental/packages/opentelemetry-sdk-metrics-base/src/ObservableBaseMetric.ts # experimental/packages/opentelemetry-sdk-metrics-base/src/ObservableResult.ts # experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts

legendecas · 2021-11-12T03:48:55Z

So we don't have an updated SDK implementation for the updated API yet and the tests for exporters are complaining about that.

pirgeo

The changes look good to me, I just wonder: was the Observation not used before?

vmarchaud · 2021-11-14T16:58:24Z

So we don't have an updated SDK implementation for the updated API yet and the tests for exporters are complaining about that.

Do we need to ignore tests from exporters while we are working on the metrics api/sdk ? cc @dyladan

legendecas · 2021-11-15T03:13:58Z

The changes look good to me, I just wonder: was the Observation not used before?

It's being used in batch observers. And batch observers were removed in #2566.

dyladan · 2021-11-17T16:47:00Z

So we don't have an updated SDK implementation for the updated API yet and the tests for exporters are complaining about that.

Do we need to ignore tests from exporters while we are working on the metrics api/sdk ? cc @dyladan

I think we should remove the exporters like we did the SDK. Let's talk about it in SIG

vmarchaud · 2021-11-20T08:22:51Z

@legendecas exporter tests are still failing for some reasons :/

legendecas · 2021-11-22T02:23:23Z

@vmarchaud updated with exporters' changes removed since they are linked to a fixed version of api-metrics.

feat(api-metrics): async instruments spec compliance

5fbb489

legendecas mentioned this pull request Nov 2, 2021

feat!(metrics): remove batch observer #2566

Merged

legendecas marked this pull request as ready for review November 3, 2021 02:31

legendecas requested a review from a team November 3, 2021 02:31

fixup!

6b30d46

dyladan reviewed Nov 3, 2021

View reviewed changes

dyladan approved these changes Nov 5, 2021

View reviewed changes

legendecas added 2 commits November 11, 2021 17:28

fixup!

740ddc5

This comment has been minimized.

Sign in to view

legendecas closed this Nov 12, 2021

legendecas reopened this Nov 12, 2021

pirgeo approved these changes Nov 12, 2021

View reviewed changes

vmarchaud added the breaking label Nov 14, 2021

Merge branch 'main' into metrics-ff/async-instruments

e996309

This comment has been minimized.

Sign in to view

Merge branch 'main' into metrics-ff/async-instruments

fa5c64c

vmarchaud and others added 2 commits November 21, 2021 21:29

Merge branch 'main' into metrics-ff/async-instruments

0c3980d

fixup! revert changes in exporters

d8a666b

vmarchaud approved these changes Nov 22, 2021

View reviewed changes

vmarchaud merged commit 5e9c7e1 into open-telemetry:main Nov 22, 2021

legendecas deleted the metrics-ff/async-instruments branch November 22, 2021 16:25

pirgeo mentioned this pull request Nov 30, 2021

REQUEST: New membership for pirgeo open-telemetry/community#926

Closed

6 tasks

dyladan mentioned this pull request Mar 16, 2022

chore: release proposal 1.1.0 #2838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api-metrics): async instruments spec compliance #2569

feat(api-metrics): async instruments spec compliance #2569

legendecas commented Oct 27, 2021 •

edited

Loading

codecov bot commented Oct 27, 2021 •

edited

Loading

dyladan left a comment •

edited

Loading

dyladan Nov 3, 2021

legendecas Nov 3, 2021

dyladan Nov 3, 2021

legendecas Nov 3, 2021

legendecas commented Nov 3, 2021

This comment has been minimized.

legendecas commented Nov 12, 2021

pirgeo left a comment

vmarchaud commented Nov 14, 2021

legendecas commented Nov 15, 2021

This comment has been minimized.

dyladan commented Nov 17, 2021

vmarchaud commented Nov 20, 2021

legendecas commented Nov 22, 2021

feat(api-metrics): async instruments spec compliance #2569

feat(api-metrics): async instruments spec compliance #2569

Conversation

legendecas commented Oct 27, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

Checklist:

codecov bot commented Oct 27, 2021 • edited Loading

Codecov Report

dyladan left a comment • edited Loading

Choose a reason for hiding this comment

dyladan Nov 3, 2021

Choose a reason for hiding this comment

legendecas Nov 3, 2021

Choose a reason for hiding this comment

dyladan Nov 3, 2021

Choose a reason for hiding this comment

legendecas Nov 3, 2021

Choose a reason for hiding this comment

legendecas commented Nov 3, 2021

This comment has been minimized.

legendecas commented Nov 12, 2021

pirgeo left a comment

Choose a reason for hiding this comment

vmarchaud commented Nov 14, 2021

legendecas commented Nov 15, 2021

This comment has been minimized.

dyladan commented Nov 17, 2021

vmarchaud commented Nov 20, 2021

legendecas commented Nov 22, 2021

legendecas commented Oct 27, 2021 •

edited

Loading

codecov bot commented Oct 27, 2021 •

edited

Loading

dyladan left a comment •

edited

Loading