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.

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

Fix collection of exponential histogram #3798

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

euroelessar
Copy link
Contributor

@euroelessar euroelessar commented Mar 20, 2024

Description

Fix various small issues in exponential histogram collection discovered by a newly added test and issues mentioned below:

  1. maintain self._scale to be consistent with current bucket's scale
  2. consistently pass scale of the correct buckets alongside the buckets to method calls
  3. maintain min, max, sum, count, zero_count values across collection calls
  4. calculate the exponential bucket's index based on the index in the circular buffer based on the index_base instead of index_start, also take wrapping around the buffer into account
  5. calculate an expected span size based on the index_start and new index instead of index_end
  6. while collecting the data point, rotate the circular buffer so that index_start matches index_base

Fixes #3393
Fixes #3600
Fixes #3562

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Fixed existing test for cumulative collection
  • Added a new pseudo-random blackbox test for validating that collected result's buckets exactly match the expected distribution

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

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

@euroelessar euroelessar requested a review from a team March 20, 2024 20:52
@rbtz-openai
Copy link
Contributor

cc: @ocelotl

@xrmx
Copy link
Contributor

xrmx commented Mar 22, 2024

Love the explanation near the code changes, thanks @euroelessar !

@ocelotl ocelotl self-assigned this Mar 22, 2024
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR.

@euroelessar
Copy link
Contributor Author

@srikanthccv thanks! can you recommend some other approver/maintainer to review this pr as well so it could be merged, please?

@srikanthccv
Copy link
Member

@ocelotl will do another review. He already assigned this to him.

@ocelotl ocelotl enabled auto-merge (squash) April 5, 2024 00:52
auto-merge was automatically disabled April 5, 2024 00:55

Pull Request is not mergeable

auto-merge was automatically disabled April 5, 2024 00:55

Pull Request is not mergeable

@ocelotl ocelotl merged commit da30869 into open-telemetry:main Apr 5, 2024
232 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants