-
Notifications
You must be signed in to change notification settings - Fork 229
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
Requesting TC review for JS metrics API/SDK spec compliance #1204
Comments
I added this to the next TC meeting agenda. |
I can do it, @dyladan @legendecas would you provide a permanent link to the doc/code? Is this about the API and SDK only, or it covers some exporters (e.g. OTLP)? |
The opentelemetry-js repo has issue templates which makes it harder to file issues found during the review, to make it easier, I'll post my findings in individual comments here. |
API not mentioned in the spec but provided by the implementation: |
API spec requires a state object to be passed to the callback, I couldn't find how it is supported in opentelemetry-js, e.g. https://github.com/open-telemetry/opentelemetry-js/blob/d154066354863e8c682b4aa488d79c7e914bf91c/api/src/metrics/Meter.ts#L110. The API SHOULD provide some way to pass state to the callback. |
Nit: not part of the spec compliance review but I noticed this comment: Do we want |
Minor: since 1.13.0, the API spec allows optional attributes in the GetMeter API: |
Exemplar in the SDK spec is not yet Stable, need to be removed or hidden if the intention is to ship the initial stable JS package https://github.com/open-telemetry/opentelemetry-js/tree/d154066354863e8c682b4aa488d79c7e914bf91c/experimental/packages/opentelemetry-sdk-metrics/src/exemplar. |
MetricReader doesn't have |
I haven't looked deeper so I could be wrong, it seems the ConsoleExporter is only supporting CUMULATIVE? https://github.com/open-telemetry/opentelemetry-js/blob/d154066354863e8c682b4aa488d79c7e914bf91c/experimental/packages/opentelemetry-sdk-metrics/src/export/ConsoleMetricExporter.ts#L41 The specification requires both CUMULATIVE and DELTA to be supported https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/stdout.md#opentelemetry-metrics-exporter---standard-output. InMemoryExporter seems to be covering both https://github.com/open-telemetry/opentelemetry-js/blob/d154066354863e8c682b4aa488d79c7e914bf91c/experimental/packages/opentelemetry-sdk-metrics/src/export/InMemoryMetricExporter.ts#L68. |
So far I've finished reviewing the following parts:
I'll cover these parts next week:
I don't plan to cover these parts since the spec is not yet marked as Stable (if a dry run would be helpful, please let me know and I can do it):
|
This is defined in the spec, with an implementation specialized name: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#multiple-instrument-callbacks. Also, the spec requires the registered callbacks to be able to be unregistered: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-counter-operations. |
It is quite easy for JavaScript to capture variable in lambdas, so by the statement in the spec:
I believe the JavaScript API is sufficient to cover the requirements. |
Exemplar is not exposed in the SDK as a public interface yet: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-metrics/src/index.ts. |
Hmm, correct. 🤔
Both the Python and Java SDK implementations seem to have solved it in the same way. May be worth opening a spec issue over if one does not yet exist. 🤔 |
I can create issues without the template so i'll move these into the JS repo |
Created issues so discussions can be moved to those. Thanks @reyang |
Requesting a compliance review from @open-telemetry/technical-committee prior to GA release. Documentation work is still underway. If there are questions you can ask myself or any of the other JS maintainers, especially @legendecas as he as done most of our metrics maintenance work or @pichlermarc since he has also done a lot of work on the metrics SDK.
The text was updated successfully, but these errors were encountered: