Skip to content
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.

Sign up for GitHub

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 2 - the accumulation and aggregator #3505

Merged
merged 12 commits into from
Mar 14, 2023
Merged

feat: exponential histogram - part 2 - the accumulation and aggregator #3505

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
be43207
feat: add exponential histogram accumulation / aggregator
mwear Dec 21, 2022
dcd4cd2
fix: apply changes picked up in rebase
mwear Jan 25, 2023
6380fe5
refactor: make InternalHistogram extend ExponentialHistogram
mwear Feb 9, 2023
0798ea1
refactor: use getters where applicable
mwear Feb 9, 2023
e48dc5b
refactor: rename pow2RoundedUp; add tsdoc; use named import
mwear Feb 10, 2023
04ae567
Merge branch 'main' into expohisto-p2-accumulation
mwear Feb 10, 2023
745aba2
Merge branch 'main' into expohisto-p2-accumulation
mwear Feb 27, 2023
bb05012
Merge branch 'main' into expohisto-p2-accumulation
mwear Mar 1, 2023
aab07fa
Apply suggestions from code review
mwear Mar 3, 2023
7f917b0
fix: update test to match comment
mwear Mar 3, 2023
b6f1cb1
Merge branch 'main' into expohisto-p2-accumulation
mwear Mar 3, 2023
51a1b23
Merge branch 'main' into expohisto-p2-accumulation
pichlermarc Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat(sdk-metrics): add exponential histogram accumulation / aggregator [#3505](https://github.com/open-telemetry/opentelemetry-js/pull/3505) @mwear
* feat (api-logs): separate Events API into its own package [3550](https://github.com/open-telemetry/opentelemetry-js/pull/3550) @martinkuba
* feat(sdk-metrics): apply binary search in histogram recording [#3539](https://github.com/open-telemetry/opentelemetry-js/pull/3539) @legendecas
* feat: support TraceState in SamplingResult [#3530](https://github.com/open-telemetry/opentelemetry-js/pull/3530) @raphael-theriault-swi
Expand Down
6 changes: 5 additions & 1 deletion experimental/packages/otlp-transformer/src/metrics/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
AggregationTemporality,
DataPoint,
DataPointType,
ExponentialHistogram,
Histogram,
MetricData,
ResourceMetrics,
Expand Down Expand Up @@ -96,7 +97,10 @@ export function toMetric(metricData: MetricData): IMetric {
}

function toSingularDataPoint(
dataPoint: DataPoint<number> | DataPoint<Histogram>,
dataPoint:
| DataPoint<number>
| DataPoint<Histogram>
| DataPoint<ExponentialHistogram>,
Comment on lines +100 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may be able to reduce this to

Suggested change
dataPoint:
| DataPoint<number>
| DataPoint<Histogram>
| DataPoint<ExponentialHistogram>,
dataPoint: DataPoint<number>,

if we change

  • toSingularDataPoints(metricData: MetricData) -> toSingularDataPoints(metricData: SumMetricData | GaugeMetricData)
  • toHistogramDataPoints(metricData: MetricData) -> toSingularDataPoints(metricData: HistogramMetricData)

This way, we don't have to export ExponentialHistogram from @opentelemetry/sdk-metrics and we can merge this PR before following through with #3654, as it would not expose any new types. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was out last week. I wanted to check in to see if this is still needed since #3654 was merged and released?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries 🙂
It's not blocking for me - since there's no release imminent I think it would be safe to merge as-is.

valueType: ValueType
) {
const out: INumberDataPoint = {
Expand Down
Loading