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

Add to_json method to ExponentialHistogram #3780

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

rbtz-openai
Copy link
Contributor

@rbtz-openai rbtz-openai commented Mar 13, 2024

Description

This fixes console exporter for ExponentialHistogram.

Fixes #3579

Type of change

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

Does This PR Require a Contrib Repo Change?

  • 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

@rbtz-openai rbtz-openai requested a review from a team March 13, 2024 20:48
Copy link

linux-foundation-easycla bot commented Mar 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

It would be nice to add a test while you are at it

@rbtz-openai
Copy link
Contributor Author

It would be nice to add a test while you are at it

I did not find a good existing place for exporter tests. Would a console exporter's integration test suffice?

@rbtz-openai
Copy link
Contributor Author

@xrmx is it changelog worthy, or should we just add "Skip Changelog"?

@xrmx
Copy link
Contributor

xrmx commented Mar 18, 2024

@xrmx is it changelog worthy, or should we just add "Skip Changelog"?

Since this is user visible I think it'll be good to have it recorded in the Changelog

@rbtz-openai
Copy link
Contributor Author

@xrmx can you please kick off the tests?

@xrmx
Copy link
Contributor

xrmx commented Mar 20, 2024

@xrmx can you please kick off the tests?

Sorry, I don't have enough powers

@rbtz-openai
Copy link
Contributor Author

@pmcollins when you have a second, can you please kick off the tests, and maybe merge if it looks good?

@rbtz-openai
Copy link
Contributor Author

@pmcollins can you please add the "Skip Public API check" label?

Currently the check thinks that:

The code in this branch removes the following public symbols:

- opentelemetry-sdk/src/opentelemetry/sdk/environment_variables.py
	OTEL_SDK_DISABLED

even though i did not touch that code.

@rbtz-openai rbtz-openai force-pushed the rbtz/expHistToJson branch 2 times, most recently from 60c7f76 to 61ebe89 Compare March 22, 2024 19:17
@rbtz-openai
Copy link
Contributor Author

@pmcollins a friendly ping =) (and maybe @srikanthccv)

@pmcollins
Copy link
Member

pmcollins commented Mar 29, 2024

Thank you for your patience :). Can you fix lint @rbtz-openai ?

@rbtz-openai
Copy link
Contributor Author

Updated, now it passes:

python scripts/eachdist.py lint --check-only

But, sadly, fails publicAPICheck

@pmcollins
Copy link
Member

Nice. That failure appears to be unrelated -- we'll have to chase it down separately.

@lzchen lzchen merged commit 5ff9046 into open-telemetry:main Apr 2, 2024
232 checks passed
ocelotl pushed a commit to SecuringCarter/opentelemetry-python that referenced this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExponentialHistogram is missing the to_json method
4 participants