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

[exporter/tanzuobservability] Refactor histogram codes #10860

Merged
merged 5 commits into from
Jul 12, 2022

Conversation

keep94
Copy link
Contributor

@keep94 keep94 commented Jun 9, 2022

Make histogram code be more like the WF proxy code.

Description:
Refactor the histogram code to make it more like the WF proxy code. Also make exponential delta histogram code more accurate.

Testing:
Unit testing.

@keep94 keep94 requested a review from a team as a code owner June 9, 2022 15:47
@keep94 keep94 requested a review from dmitryax June 9, 2022 15:47
@project-bot project-bot bot added this to In progress in Collector Jun 9, 2022
@keep94 keep94 marked this pull request as draft June 9, 2022 15:48
@keep94 keep94 marked this pull request as ready for review June 27, 2022 22:32
@dmitryax
Copy link
Member

dmitryax commented Jul 6, 2022

@open-telemetry/collector-approvers anyone else review this please?

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Other than the changelog comment, this looks good to me from a structural level. The component's code has been validated by code owners.

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@
- `prometheusreceiver`: Add `target_info` labels to resource attributes. (#11034)
- `saphanareceiver`: Fix component memory query, add better error handling (#11507)
- `sapmexporter`: Add config option to log responses from Splunk APM. (#11425)
- `tanzuobservabilityexporter`: Improve alorithm to translate OTEL delta exponential histograms into tanzu observability histograms (#10860)
Copy link
Member

Choose a reason for hiding this comment

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

We've recently changed how changelog entries are handled. Please, check the contribution guidelines, but in short, you'll just need to add a new file to ./unreleased.

@keep94
Copy link
Contributor Author

keep94 commented Jul 7, 2022

Thanks @jpkrohling I added a .yaml file under the unreleased directory for CHANGELOG.md.

@keep94
Copy link
Contributor Author

keep94 commented Jul 8, 2022

Hi. Before the build was green, but since I added a .yaml file for the change log, one of the load tests is failing. Could it be a flaky test?

@jpkrohling
Copy link
Member

Could it be a flaky test?

Yes

@keep94
Copy link
Contributor Author

keep94 commented Jul 12, 2022

Hi @jpkrohling, I fixed the misspelling that you caught in my CHANGELOG entry. Good catch by the way. Please reach out if there is anything else. Thanks.

Collector automation moved this from In progress to Reviewer approved Jul 12, 2022
@jpkrohling jpkrohling merged commit 71aedd8 into open-telemetry:main Jul 12, 2022
@keep94 keep94 deleted the 29269_and_29262 branch July 14, 2022 16:50
atoulme pushed a commit to atoulme/opentelemetry-collector-contrib that referenced this pull request Jul 16, 2022
…y#10860)

* [exporter/tanzuobservability] Refactor histogram codes

Make histogram code be more like the WF proxy code.

* Get rid of redundant comments.

* Enhance accuracy of delta histograms.

* Switch to new method of updating CHANGELOG.md

* Fix misspelling in CHANGELOG
ag-ramachandran referenced this pull request in ag-ramachandran/opentelemetry-collector-contrib Sep 15, 2022
* [exporter/tanzuobservability] Refactor histogram codes

Make histogram code be more like the WF proxy code.

* Get rid of redundant comments.

* Enhance accuracy of delta histograms.

* Switch to new method of updating CHANGELOG.md

* Fix misspelling in CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

None yet

6 participants