-
Notifications
You must be signed in to change notification settings - Fork 485
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
refactor(layers/prometheus): rewrite prometheus layer based on observe mod #5069
refactor(layers/prometheus): rewrite prometheus layer based on observe mod #5069
Conversation
ce34e6a
to
b7ea2b9
Compare
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.
Thanks a lot for this! PrometheusClientLayer doesn't have the same feature set, we can add them in following PRs.
core/src/layers/prometheus.rs
Outdated
Err(err) | ||
} | ||
if error { | ||
names.push(observe::LABEL_ERROR); |
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.
Is the order significant? Should we place path
before error
?
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.
The order of path
and error
is not important, what is important is that the order of label names and label values should correspond, otherwise there will be problems
self.stats.increment_errors_total(self.op, err.kind()); | ||
Err(err) | ||
} | ||
if path_label_level > 0 { |
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.
Let's put path
before error
?
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.
Done, how about now
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.
Thanks!
Which issue does this PR close?
related PR: #5064
Rationale for this change
What changes are included in this PR?
rewrite
PrometheusLayer
based onobserve
modAre there any user-facing changes?
API breaking change