Skip to content

Global error handler cleanup - Counter and Observable counter creation #2234

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 7 commits into from
Oct 24, 2024
Merged

Global error handler cleanup - Counter and Observable counter creation #2234

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a0e56c
internal-log counter and observable counter
lalitb Oct 24, 2024
47266ef
comment.
lalitb Oct 24, 2024
6c6d2d5
spell
lalitb Oct 24, 2024
35e62fb
Merge branch 'main' into instrument-creation-internal-log
lalitb Oct 24, 2024
2b71415
comment
lalitb Oct 24, 2024
ed5bedf
comment
lalitb Oct 24, 2024
d2d462f
comment
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
41 changes: 35 additions & 6 deletions opentelemetry-sdk/src/metrics/meter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use opentelemetry::{
InstrumentProvider, MetricsError, ObservableCounter, ObservableGauge,
ObservableUpDownCounter, Result, UpDownCounter,
},
otel_error,
};

use crate::instrumentation::Scope;
Expand Down Expand Up @@ -75,14 +76,20 @@ impl SdkMeter {
{
let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit);
if let Err(err) = validation_result {
global::handle_error(err);
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from this counter will be ignored.",
reason = format!("{}", err)
);
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:
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from this instrument will be ignored."
reason = fmt(err))

^ I prefer this version. Please see if this is better for end users.

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.

Utkarsh comment:
change
"Measurements from this instrument will be ignored." to
""Measurements from this counter will be ignored.",

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.

2024-10-24T05:32:23.524520Z ERROR main opentelemetry_sdk meter_name="mylibraryname" instrument_name="-my_counter" Measurements from this counter will be ignored. reason="Invalid instrument configuration: instrument name must start with an alphabetic character"

^ This is what we get when I used fmt module. The reason is a slightly extra verbose:
Current: Invalid instrument configuration: instrument name must start with an alphabetic character
Proposal: Instrument name must start with an alphabetic character

This is a minor thing we can change in the future.

return Counter::new(Arc::new(NoopSyncInstrument::new()));
}

match resolver
.lookup(
InstrumentKind::Counter,
builder.name,
builder.name.clone(),
builder.description,
builder.unit,
None,
Expand All @@ -91,7 +98,13 @@ impl SdkMeter {
{
Ok(counter) => counter,
Err(err) => {
global::handle_error(err);
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from this counter will be ignored.",
reason = format!("{}", err)
);
Counter::new(Arc::new(NoopSyncInstrument::new()))
}
}
Expand All @@ -107,19 +120,30 @@ impl SdkMeter {
{
let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit);
if let Err(err) = validation_result {
global::handle_error(err);
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from this observable counter will be ignored.",
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.

nit: if we are doing different message for each kind of instrument, then for observable, we need to call out that the registered callbacks will not be invoked.

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.

This is good point. Have updated it - Callbacks for this observable counter will not be invoked.

reason = format!("{}", err));
return ObservableCounter::new();
}

match resolver.measures(
InstrumentKind::ObservableCounter,
builder.name,
builder.name.clone(),
builder.description,
builder.unit,
None,
) {
Ok(ms) => {
if ms.is_empty() {
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from this observable counter will be ignored. Check View Configuration."
);
return ObservableCounter::new();
}

Expand All @@ -134,7 +158,12 @@ impl SdkMeter {
ObservableCounter::new()
}
Err(err) => {
global::handle_error(err);
otel_error!(
name: "InstrumentCreationFailed",
meter_name = self.scope.name.as_ref(),
instrument_name = builder.name.as_ref(),
message = "Measurements from the observable counter will be ignored.",
reason = format!("{}", err));
ObservableCounter::new()
}
}
Expand Down
Loading