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/logging] log min and max for histograms #5520

Merged
merged 7 commits into from
Jun 14, 2022

Conversation

pichlermarc
Copy link
Member

Description:
The logging exporter did not show min and max of histograms. This PR adds logging for min and max on both Histograms and Exponential Histograms.

Link to tracking Issue: Fixes #5518

Testing:

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #5520 (ba173e8) into main (277f98e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5520      +/-   ##
==========================================
+ Coverage   90.87%   90.88%   +0.01%     
==========================================
  Files         191      191              
  Lines       11362    11378      +16     
==========================================
+ Hits        10325    10341      +16     
  Misses        812      812              
  Partials      225      225              
Impacted Files Coverage Δ
internal/otlptext/databuffer.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 277f98e...ba173e8. Read the comment docs.

@pichlermarc pichlermarc changed the title fix(exporter/logging): log min and max for histograms. [exporter/logging] log min and max for histograms Jun 13, 2022
Comment on lines 121 to 122
b.logEntry("Min: %f", p.Min())
b.logEntry("Max: %f", p.Max())
Copy link
Member

Choose a reason for hiding this comment

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

if p.HasMin() {
b.logEntry("Min: %f", p.Min())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 66405f1. Thanks for the feedback! 🙂
I also added a check for sum as it is optional too in OTLP v0.17, and added min and max to the test data in 4eb70cd.

@pichlermarc pichlermarc marked this pull request as ready for review June 13, 2022 15:41
@pichlermarc pichlermarc requested review from a team and bogdandrutu June 13, 2022 15:41
@@ -146,6 +157,14 @@ func (b *dataBuffer) logExponentialHistogramDataPoints(ps pmetric.ExponentialHis
b.logEntry("Count: %d", p.Count())
b.logEntry("Sum: %f", p.Sum())
Copy link
Contributor

Choose a reason for hiding this comment

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

Sum should be optional for exponential histogram as well. I'll open an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that #5530 merged, please update the PR to check HasSum here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 2883283. Thanks for updating the proto version! 🙂

@pichlermarc
Copy link
Member Author

pichlermarc commented Jun 13, 2022

go 1.18 unit tests are currently failing. I'm investigating.
Edit: tests are green again. 🙂

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this and following up after my change!

@codeboten codeboten merged commit e5ac6d2 into open-telemetry:main Jun 14, 2022
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.

[exporter/logging] Histogram min and max fields are not logged.
3 participants