feat(sdk-metrics-base): implement async instruments support #2686
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2686 +/- ##
==========================================
+ Coverage 93.04% 93.22% +0.17%
==========================================
Files 155 157 +2
Lines 5378 5432 +54
Branches 1137 1141 +4
==========================================
+ Hits 5004 5064 +60
+ Misses 374 368 -6
|
# Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
|
This PR is ready for review :) |
experimental/packages/opentelemetry-sdk-metrics-base/src/view/AttributesProcessor.ts
Show resolved
Hide resolved
| ): Promise<Maybe<MetricData>> { | ||
| const observableResult = new ObservableResult(); | ||
| // TODO: timeout with callback | ||
| await this._callback(observableResult); |
There was a problem hiding this comment.
Will the timeout be added in a follow-up PR? callWithTimeout() is available in utils.ts, however I'm not too sure what an appropriate timeout would be. 🤔 I checked the Java SDK and it does not seem like they handle timeouts there.
There was a problem hiding this comment.
I'd believe so. Since MetricReader.collect already has a timeout option, I'd believe the timeout should be applied on async metric observations, as the timeout is not reasonable for synchronous instrumentation in JavaScript (we can't cancel or interrupt sync operations with pure JavaScript) and other parts of a collection (they are all synchronous).
| export class ObservableResult implements metrics.ObservableResult { | ||
| buffer = new AttributeHashMap<number>(); | ||
|
|
||
| observe(value: number, attributes: metrics.Attributes = {}): void { |
There was a problem hiding this comment.
Should we document that if more than one measurement with same attributes is observed in one collection cycle then last one is kept?
Which problem is this PR solving?
This is the continuation part of #2636 for async instruments metric streams.
This is a prototype implementation described in https://docs.google.com/document/d/1EluDhj5UanNATS3hR8xx30sv55Se7uYjsGYslvNWHtc/edit#.
Short description of the changes
Pending spec clarifications:
Depends on:
Type of change
How Has This Been Tested?
Checklist: