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.

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

Jump to bottom

Global error handler cleanup - PeriodicReader #2243

Merged
merged 4 commits into from
Oct 25, 2024
Merged

Global error handler cleanup - PeriodicReader #2243

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59e28e2
initial commit
lalitb Oct 24, 2024
0e26b83
Fix
lalitb Oct 24, 2024
b9a612d
Merge branch 'main' into periodicreader-internal-log
lalitb Oct 24, 2024
387c810
Merge branch 'main' into periodicreader-internal-log
cijothomas Oct 25, 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
37 changes: 20 additions & 17 deletions opentelemetry-sdk/src/metrics/periodic_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
StreamExt,
};
use opentelemetry::{
global,
metrics::{MetricsError, Result},
otel_error,
otel_debug, otel_error,
};

use crate::runtime::Runtime;
Expand Down Expand Up @@ -245,34 +244,39 @@
Either::Left((res, _)) => {
res // return the status of export.
}
Either::Right(_) => {
otel_error!(
name: "collect_and_export",
status = "timed_out"
);
Err(MetricsError::Other("export timed out".into()))
}
Either::Right(_) => Err(MetricsError::Other("export timed out".into())),

Check warning on line 247 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L247 

Added line #L247 was not covered by tests
}
}

async fn process_message(&mut self, message: Message) -> bool {
match message {
Message::Export => {
if let Err(err) = self.collect_and_export().await {
global::handle_error(err)
otel_error!(

Check warning on line 255 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L255 

Added line #L255 was not covered by tests
name: "PeriodicReader.ExportFailed",
message = "Failed to export metrics",
reason = format!("{}", err));

Check warning on line 258 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L258 

Added line #L258 was not covered by tests
}
}
Message::Flush(ch) => {
let res = self.collect_and_export().await;
if ch.send(res).is_err() {
global::handle_error(MetricsError::Other("flush channel closed".into()))
if let Err(send_error) = ch.send(res) {
otel_debug!(

Check warning on line 264 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L264 

Added line #L264 was not covered by tests
name: "PeriodicReader.Flush.SendResultError",
message = "Failed to send flush result",
reason = format!("{:?}", send_error),

Check warning on line 267 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L267 

Added line #L267 was not covered by tests
);
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 doesn't log actual flush error, but failure to send the result status to the channel. So keeping it debug. It would be good to also log the actual flush status as attribute here, but that would need us to clone before it is sent to channel.

}
}
Message::Shutdown(ch) => {
let res = self.collect_and_export().await;
let _ = self.reader.exporter.shutdown();
if ch.send(res).is_err() {
global::handle_error(MetricsError::Other("shutdown channel closed".into()))
if let Err(send_error) = ch.send(res) {
otel_debug!(

Check warning on line 275 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L275 

Added line #L275 was not covered by tests
name: "PeriodicReader.Shutdown.SendResultError",
message = "Failed to send shutdown result",
reason = format!("{:?}", send_error),

Check warning on line 278 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L278 

Added line #L278 was not covered by tests
);
}
return false;
}
Expand Down Expand Up @@ -300,9 +304,8 @@
let worker = match &mut inner.sdk_producer_or_worker {
ProducerOrWorker::Producer(_) => {
// Only register once. If producer is already set, do nothing.
global::handle_error(MetricsError::Other(
"duplicate meter registration, did not register manual reader".into(),
));
otel_debug!(name: "PeriodicReader.DuplicateRegistration",
message = "duplicate registration found, did not register periodic reader.");

Check warning on line 308 in opentelemetry-sdk/src/metrics/periodic_reader.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/periodic_reader.rs#L307-L308 

Added lines #L307 - L308 were not covered by tests
return;
}
ProducerOrWorker::Worker(w) => mem::replace(w, Box::new(|_| {})),
Expand Down
Loading