fix(sdk-metrics-base): combine concurrent observable callback invocations #2785
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2785 +/- ##
==========================================
- Coverage 93.43% 93.42% -0.01%
==========================================
Files 159 161 +2
Lines 5452 5494 +42
Branches 1145 1149 +4
==========================================
+ Hits 5094 5133 +39
- Misses 358 361 +3
|
| @@ -80,6 +80,9 @@ export interface Meter { | |||
|
|
|||
| /** | |||
| * Creates a new `ObservableGauge` metric. | |||
| * | |||
| * The callback SHOULD not be called concurrently. | |||
There was a problem hiding this comment.
This sounds like something we are telling the user not to do when it is the SDK that actually calls the callback
There was a problem hiding this comment.
Yeah, maybe we can update the words to be like "The callback is been called only when it finishes the previous invocation or timeouts."
| const viewDescriptor = createInstrumentDescriptorWithView(view, descriptor); | ||
| const aggregator = view.aggregation.createAggregator(viewDescriptor); | ||
| const storage = new SyncMetricStorage(viewDescriptor, aggregator, view.attributesProcessor); | ||
| // TODO: handle conflicts |
There was a problem hiding this comment.
Is there an issue for this TODO so it doesn't get forgotten
There was a problem hiding this comment.
It is being worked at #2734
|
|
||
| constructor(public resource: Resource) {} | ||
|
|
||
| getMeterSharedState(instrumentationLibrary: InstrumentationLibrary) { | ||
| // TODO: meter identity |
There was a problem hiding this comment.
Do we have an issue for meter identity that can be referenced here so it isn't forgotten?
There was a problem hiding this comment.
I'll update the comment to include a reference to #2593.
| const promise = Promise.all(Array.from(this._callbackMap.keys()).map(observableCallback => { | ||
| const observableResult = new ObservableResult(); | ||
| this._observableResultMemo.set(observableCallback, observableResult); | ||
| // TODO: timeout with callback |
There was a problem hiding this comment.
I'm not sure what this TODO means. Can you open an issue for it and reference it in the comment or make the comment more descriptive?
There was a problem hiding this comment.
We have a clarification request on spec at open-telemetry/opentelemetry-specification#2295 for this problem.
I'll update the comment.
|
The spec has open-telemetry/opentelemetry-specification#2361 to define that:
We should follow the latest definition. Closing for now. |
Which problem is this PR solving?
As https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-counter-creation states:
This PR documents that the async observable callbacks SHOULD not be called concurrently in JavaScript.
When multiple (or single) metric readers initiate
collectoperation concurrently, the invocation of the callback of async observables is merged.Fixes #2782
Short description of the changes
ObservableRegistryto combineobservableCallbacks for multiple async metrics.ObservableCallbacks at once before metric collections.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: