Skip to content

Global error handler cleanup - exponential histogram #2235

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 24, 2024
Merged

Global error handler cleanup - exponential histogram #2235

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d48133f
metrics-sdk-error
lalitb Oct 24, 2024
502885e
Merge branch 'main' into exp-hist-internal-log
lalitb Oct 24, 2024
76a02a1
Merge branch 'main' into exp-hist-internal-log
TommyCpp Oct 24, 2024
bde6dd1
Update opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
lalitb Oct 24, 2024
4bab7f5
review comment
lalitb Oct 24, 2024
0a39b3b
Merge branch 'main' into exp-hist-internal-log
lalitb Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{collections::HashMap, f64::consts::LOG2_E, sync::Mutex, time::SystemTime};

use once_cell::sync::Lazy;
use opentelemetry::{metrics::MetricsError, KeyValue};
use opentelemetry::{otel_debug, KeyValue};

use crate::{
metrics::data::{self, Aggregation, Temporality},
Expand Down Expand Up @@ -100,9 +100,20 @@ impl<T: Number> ExpoHistogramDataPoint<T> {
if (self.scale - scale_delta as i8) < EXPO_MIN_SCALE {
// With a scale of -10 there is only two buckets for the whole range of f64 values.
// This can only happen if there is a max size of 1.
opentelemetry::global::handle_error(MetricsError::Other(
"exponential histogram scale underflow".into(),
));

// 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

@TommyCpp TommyCpp Oct 24, 2024

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 •
edited
Loading

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

@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.

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

Copy link
Member

@cijothomas cijothomas Oct 25, 2024

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)

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

@cijothomas cijothomas Oct 24, 2024

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

@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.

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

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

@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.

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

@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.

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


return;
}
// Downscale
Expand Down
Loading