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

Jump to bottom

Fix a few ExplicitBucketHistogram issues #2377

Merged
merged 5 commits into from
Jan 18, 2022
Merged

Fix a few ExplicitBucketHistogram issues #2377

merged 5 commits into from
Jan 18, 2022

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jan 14, 2022 •
edited
Loading

Description

Fixes a few things in ExplicitBucketHistogramAggregation:

  • rename value to _bucket_counts to match OTLP. I would rename boundaries as well, but that is different in the SDK spec than the data model for some reason.
  • Update boundaries param to accept floating point (see spec). Make the member an immutable tuple
  • Remove inf from boundaries, it is implicit in the data model.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tests are passing

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@aabmass aabmass force-pushed the remove-inf branch 2 times, most recently from 94d305d to e401100 Compare January 14, 2022 22:20
@aabmass aabmass marked this pull request as ready for review January 14, 2022 22:26
@aabmass aabmass requested review from a team and ocelotl January 14, 2022 22:26
@aabmass aabmass added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 14, 2022
@aabmass aabmass added metrics sdk Affects the SDK package. labels Jan 18, 2022
@ocelotl ocelotl merged commit 7d076cc into open-telemetry:main Jan 18, 2022
@aabmass aabmass deleted the remove-inf branch January 18, 2022 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@ocelotl ocelotl ocelotl approved these changes

@lzchen lzchen Awaiting requested review from lzchen

@codeboten codeboten Awaiting requested review from codeboten

Assignees
No one assigned
Labels
metrics sdk Affects the SDK package. Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aabmass @ocelotl @srikanthccv