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.

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!(metrics): remove batch observer #2566

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Oct 27, 2021

Fixes #2565

This removes the batch observer which was removed from the API in order to simplify the 1.0 release

@dyladan dyladan requested a review from a team October 27, 2021 14:57
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #2566 (7f11892) into main (5bb8322) will increase coverage by 0.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
+ Coverage   92.71%   93.00%   +0.28%     
==========================================
  Files         130      138       +8     
  Lines        4478     5092     +614     
  Branches      966     1095     +129     
==========================================
+ Hits         4152     4736     +584     
- Misses        326      356      +30     
Impacted Files Coverage Δ
...metry-sdk-metrics-base/src/ObservableBaseMetric.ts 90.90% <0.00%> (-9.10%) ⬇️
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts 87.50% <0.00%> (-1.10%) ⬇️
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <0.00%> (ø)
...emetry-sdk-metrics-base/src/BatchObserverResult.ts
...pentelemetry-sdk-metrics-base/src/BatchObserver.ts
...exporter-metrics-otlp-http/src/transformMetrics.ts 95.65% <0.00%> (ø)
...opentelemetry-exporter-trace-otlp-http/src/util.ts 100.00% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 96.98% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
... and 6 more

Copy link
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

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM

@obecny
Copy link
Member

obecny commented Oct 27, 2021

The thing is it wasn't either removed or specified yet and it is quite useful when you have to update many observable metrics at the same time. If we remove it we will have to implement some caching mechanism to avoid cpu time consuming calculations or IO operations. Currently we can read async stats from certain library and update as much observables as we want. Without this it would be more complicated. Anyway I'm not blocking that if that is the way to go, you have been made aware of this :).

@legendecas
Copy link
Member

I would also +1 for BatchObserver to be part of v1 of metrics API, it is very useful when a single observation resulted in a series of observable updates. But I'm not against removing it if the spec team is determined to remove it from v1.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

@vmarchaud
Copy link
Member

Well if its not in the v1 of the metrics spec, we can't have it if we want to release 1.0 right ? That would means we declare stable a implementation that isn't frozen at the spec level i believe

@weyert
Copy link
Contributor

weyert commented Oct 30, 2021

I think we could package it up in a separate extras/Utils package? But I agree if it’s not part of v1 it shouldn’t be part of it. Similarly there are also no sync updown counters either; requiring me to write a sync wrapper for the the asynchronous one.

@dyladan
Copy link
Member Author

dyladan commented Nov 1, 2021

Brought this up in the maintainers SIG this morning. They suggested we can either remove the feature or keep it by somehow marking the feature as experimental in some way so that it is not stable but doesn't affect metrics api 1.0 stability.

Unfortunately there is no jsdoc tag for this, but we could probably find another way.

@vmarchaud
Copy link
Member

Could we just let it inside this package and not move it to the api repo afterwards ?

@legendecas
Copy link
Member

The problem with the batch observer API is that the current API spec for creating an async instrument requires a callback since every async instrument has to know how to collect stats. However, the callback is optional in the current SDK, so that we can collect stats in the batch observer's callback instead of the callbacks of each async instrument. So things might get confusing for users if we are going forward without the batch observer being removed:

const observable = meter.createObservableGauge('test', () => { /* a noop callback is required */ });
const observer = meter.createBatchObserver(result => {
  // a second callback!
  result.observe({}, [ observable.observation(0) ]);
});

Notably, BatchObserverResult has to be working with observable.observation, and it is part of the Observable API so if we're going to keep the BatchObserver then all SDKs have to implement the method. I'd believe this is not promising.

I'm still leaning on hard compliance with API spec and marking the callback parameter as required (#2569) and removing all batch observer stuff if it is determined.

@dyladan
Copy link
Member Author

dyladan commented Nov 2, 2021

The problem with the batch observer API is that the current API spec for creating an async instrument requires a callback since every async instrument has to know how to collect stats. However, the callback is optional in the current SDK, so that we can collect stats in the batch observer's callback instead of the callbacks of each async instrument. So things might get confusing for users if we are going forward without the batch observer being removed:

const observable = meter.createObservableGauge('test', () => { /* a noop callback is required */ });
const observer = meter.createBatchObserver(result => {
  // a second callback!
  result.observe({}, [ observable.observation(0) ]);
});

Notably, BatchObserverResult has to be working with observable.observation, and it is part of the Observable API so if we're going to keep the BatchObserver then all SDKs have to implement the method. I'd believe this is not promising.

I'm still leaning on hard compliance with API spec and marking the callback parameter as required (#2569) and removing all batch observer stuff if it is determined.

I agree with you here. It doesn't solve the problem about atomic/transactional changes but I believe the batch observer API will be different than we have it here so it will need to be changed anyways, and it complicates the SDK quite a bit so removing it will make the SDK implementation changes simpler I believe.

@dyladan dyladan merged commit 0d0b668 into open-telemetry:main Nov 2, 2021
@dyladan dyladan deleted the remove-batch branch November 2, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api-metrics]: Remove BatchObserver
6 participants