-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
feat: add metrics for failed requests #414
Conversation
let registry = registry.unwrap_or_else(instantiate_registry); | ||
register_custom_metrics(®istry); | ||
instantiate_prometheus_metrics_handler(registry) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed to avoid SetGlobalDefaultError("a global default trace dispatcher has already been set")
, since the normal instantiate
runs instantiate_tracing_and_logging(log_format);
, which includes tracing::subscriber::set_global_default(collector).unwrap();
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 - great catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some suggestions. The I think you should consider doing the match/map chain but the logic looks good to me and the tests seem to do what they need!
let metrics_output = String::from_utf8(buffer).unwrap(); | ||
|
||
let value_ok = parse_metrics_for_status_code(&metrics_output, 200).expect("Metric with status code 200 not found"); | ||
assert_eq!(value_ok, -1.0, "Metric value for status code 200 did not match expected"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why -1.0 and not 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and same thing for the other values. I see we reduce by -1.0 once it's done, I'm expecting the active_requests to fluctuate between 0 and however many simultaneous connections we can handle. so it being -1.0
would mean it only went through the it's done
branch instead of both the started
and it's done
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds metrics for failed requests.
Closes #412