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

Global error handler cleanup - Metrics View #2249

Merged
merged 8 commits into from
Oct 26, 2024
+5 −16
Merged

Global error handler cleanup - Metrics View #2249

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9afda7b
initial commit
lalitb Oct 25, 2024
595181c
fix
lalitb Oct 25, 2024
cd9dd7d
Merge branch 'main' into view-internal-log
lalitb Oct 25, 2024
70ab680
Update view.rs
lalitb Oct 26, 2024
f112259
Update view.rs
lalitb Oct 26, 2024
940cb5a
Merge branch 'main' into view-internal-log
lalitb Oct 26, 2024
707a34e
Update view.rs
lalitb Oct 26, 2024
cd57696
lint error
lalitb Oct 26, 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
31 changes: 20 additions & 11 deletions opentelemetry-sdk/src/metrics/view.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use super::instrument::{Instrument, Stream};
use glob::Pattern;
use opentelemetry::{
global,
metrics::{MetricsError, Result},
otel_warn,
};

fn empty_view(_inst: &Instrument) -> Option<Stream> {
Expand Down Expand Up @@ -102,19 +102,25 @@
/// ```
pub fn new_view(criteria: Instrument, mask: Stream) -> Result<Box<dyn View>> {
if criteria.is_empty() {
global::handle_error(MetricsError::Config(format!(
"no criteria provided, dropping view. mask: {mask:?}"
)));
otel_warn!(

Check warning on line 105 in opentelemetry-sdk/src/metrics/view.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/view.rs#L105 

Added line #L105 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

for a method that returns Result, I don't think there is any need of internal logging. If we really want to do some log, then it should be Debug level.

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual error is getting lost in all these cases, as we just return success with empty view.

Copy link
Member

Choose a reason for hiding this comment

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

if we always return success, then that looks like a flaw. Since it is not introduced in this PR, lets remove the error logging from here, and put a todo (optional). We need to rework the View and can fix these then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TODO, and removed logging.

name: "View.NoCriteriaProvided",
message= "no criteria provided, dropping view",
criteria = format!("{:?}", criteria),
mask = format!("{:?}", mask),

Check warning on line 109 in opentelemetry-sdk/src/metrics/view.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/view.rs#L108-L109 

Added lines #L108 - L109 were not covered by tests
);
return Ok(Box::new(empty_view));
}
let contains_wildcard = criteria.name.contains(['*', '?']);
let err_msg_criteria = criteria.clone();

let match_fn: Box<dyn Fn(&Instrument) -> bool + Send + Sync> = if contains_wildcard {
if mask.name != "" {
global::handle_error(MetricsError::Config(format!(
"name replacement for multiple instruments, dropping view, criteria: {criteria:?}, mask: {mask:?}"
)));
otel_warn!(

Check warning on line 118 in opentelemetry-sdk/src/metrics/view.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/view.rs#L118 

Added line #L118 was not covered by tests
name: "View.NameReplacementMultipleInstruments",
message = "name replacement for multiple instruments, dropping view",
criteria = format!("{:?}", criteria),
mask = format!("{:?}", mask),

Check warning on line 122 in opentelemetry-sdk/src/metrics/view.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/view.rs#L121-L122 

Added lines #L121 - L122 were not covered by tests
);
return Ok(Box::new(empty_view));
}

Expand All @@ -138,10 +144,13 @@
match ma.validate() {
Ok(_) => agg = Some(ma.clone()),
Err(err) => {
global::handle_error(MetricsError::Other(format!(
"{}, proceeding as if view did not exist. criteria: {:?}, mask: {:?}",
err, err_msg_criteria, mask
)));
otel_warn!(
name: "View.AggregationValidationFailed",
message = "failed to validate aggregation, proceeding as if view did not exist",
criteria = format!("{:?}", err_msg_criteria),
mask = format!("{:?}", mask),
reason = format!("{:?}", err),

Check warning on line 152 in opentelemetry-sdk/src/metrics/view.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/metrics/view.rs#L150-L152 

Added lines #L150 - L152 were not covered by tests
);
return Ok(Box::new(empty_view));
}
}
Expand Down
Loading