Skip to content

Conversation

@lalitb
Copy link
Member

@lalitb lalitb commented Oct 24, 2024

Fixes #
Design discussion issue (if applicable) #

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team as a code owner October 24, 2024 04:37
record_sum = self.record_sum,
value = format!("{:?}", v),
error = "The measurement will be dropped due to scale underflow. Check the histogram configuration"
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Cijo's comment:
Unless we are 100% sure this cannot cause flooding of logs, lets remove the log from here, and leave a TODO to add logging once we understand more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to log as otel_debug till we are not sure.

@codecov
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.5%. Comparing base (0202299) to head (0a39b3b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...-sdk/src/metrics/internal/exponential_histogram.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2235   +/-   ##
=====================================
  Coverage   79.5%   79.5%           
=====================================
  Files        121     121           
  Lines      21113   21112    -1     
=====================================
  Hits       16803   16803           
+ Misses      4310    4309    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scale_delta = scale_delta,
max_size = self.max_size,
min_scale = EXPO_MIN_SCALE,
record_min_max = self.record_min_max,
Copy link
Member

Choose a reason for hiding this comment

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

these configs are too much information, so better skip it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think max_size, min_scale, value and message should be kept, rest can be removed.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Okay to merge as exponential histogram is unlikely to declared stable soon (as it requires views), so we can revisit the comments later.


// TODO - to check if this should be logged as an error if this is auto-recoverable.
otel_debug!(
name: "ExponentialHistogramDataPoint.Scale.Underflow",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the naming convention is CamelCase. for log names. Is there a reason here?

Copy link
Member Author

@lalitb lalitb Oct 24, 2024

Choose a reason for hiding this comment

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

There are no specific reasons; it simply offers clearer visibility and effectively represents hierarchy when combined with dots. For example, ExponentialHistogramDataPoint.Scale.Underflow seems better to exponentialhistogramdatapoint.scale.underflow or exponential_histogram_datapoint.scale.underflow.

Or better example to represent hierarch would be LoggerProvider.Drop.ShutdownMutexPoisoned as compared to loggerprovider.drop.shutdownmutexpoisoned
But happy to change if there are specific guidelines/preferences.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TommyCpp - I am merging this to unblock. If there are specific recommendations, we can cover separately as this will affect throughout.

Copy link
Member

Choose a reason for hiding this comment

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

We can get some inputs from OTel Events team recommendations. Our event name is same as what OTel Events are. (There are lot of ongoing discussions in this area, so we need to comeback to this once things are more settled)

@lalitb lalitb merged commit a18853e into open-telemetry:main Oct 24, 2024
23 of 25 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

Development

Successfully merging this pull request may close these issues.

3 participants