-
Notifications
You must be signed in to change notification settings - Fork 835
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(sdk-metrics-base): document and export basic APIs #2725
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2725 +/- ##
==========================================
+ Coverage 93.23% 93.26% +0.03%
==========================================
Files 158 158
Lines 5436 5434 -2
Branches 1141 1141
==========================================
Hits 5068 5068
+ Misses 368 366 -2
|
import { View } from './view/View'; | ||
|
||
/** | ||
* Supported types of metric instruments. | ||
* @see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better to point to a tag/commit instead main which moves day by day.
applies also to other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, like said, the spec is being updated day by day and it has not been released with a "tagged" version yet. I'd remove these links for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 👍
Just two nits to make the use of '.' more consistent, feel free to ignore :)
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricData.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricData.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with Flarna that we should pin the version of the spec that the SDK target, may be worth to add it inside the readme too (a beginning of compat matrix) otherwise lgtm
Which problem is this PR solving?
Document and export basic public APIs for implementing an exporter.
Type of change
Checklist:
Unit tests have been added