-
Notifications
You must be signed in to change notification settings - Fork 821
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: exponential histogram - part 3 - export #3506
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3506 +/- ##
==========================================
+ Coverage 93.61% 93.66% +0.04%
==========================================
Files 274 277 +3
Lines 8197 8462 +265
Branches 1691 1756 +65
==========================================
+ Hits 7674 7926 +252
- Misses 523 536 +13
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
b4a7d4d
to
9cf2171
Compare
CHANGELOG.md
Outdated
* feat(sdk-metrics): exponential histogram export [#3506](https://github.com/open-telemetry/opentelemetry-js/pull/3506) @mwear | ||
* feat(sdk-metrics): add exponential histogram accumulation / aggregator [#3505](https://github.com/open-telemetry/opentelemetry-js/pull/3505) @mwear |
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.
Can probably combine these changelog entries into one more user-friendly one that references both PRs since this release actually makes this feature available.
@@ -184,7 +184,7 @@ export interface IExponentialHistogramDataPoint { | |||
startTimeUnixNano?: number; | |||
|
|||
/** ExponentialHistogramDataPoint timeUnixNano */ | |||
timeUnixNano?: string; | |||
timeUnixNano?: number; |
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.
Was this a mistake that got through before?
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.
This was an existing issue on main that predates my work on the exponential histogram.
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.
Looks great, thanks for putting in all the effort to implement this. 🙂🚀
Failing tests are unrelated to this PR, see #3700 |
Which problem is this PR solving?
This PR is part 3 in a series of PRs to add exponential histogram support. See this comparison introduced by this PR (omitting those from Part 1 and Part 2).
Partially Fixes #3324
Short description of the changes
This PRs addresses the final details to wire up exponential histogram export to OTLP.
This code is heavily based on the Golang reference implementation. For other details see:
For the other PRs in this series see:
You can see all 3 PRs combined in the original draft PR
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: