-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
WIP: feat(metrics): add actix-web metrics exporter #793
base: main
Are you sure you want to change the base?
Conversation
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 looks fine, and I read your discussion in nlopes/actix-web-prom#72 - but still not certain I understood the resolution of the problem -- would it make sense for the middleware to have an extra API to override path param to convert from /srcA,srcB/1/1/1
to srcA,srcB
(but at the same time ignore the tilejson request /srcA,srcB
? Should this be some magical attribute?
|
||
// labels.insert("label_foo".to_string(), "value_barbarbbabbmiles".to_string()); | ||
let prometheus = PrometheusMetricsBuilder::new("martin") | ||
.endpoint("/metrics") |
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.
if we start handling /metrics
, we would have to declare this as a reserved keyword (we might be missing docs in a martin book for this). One workaround perhaps would be to add everything "magical" to the /_/metrics
style prefixes because _
is not a valid source ID i think.
martin/martin/src/srv/server.rs
Lines 32 to 37 in a2acd17
/// List of keywords that cannot be used as source IDs. Some of these are reserved for future use. | |
/// Reserved keywords must never end in a "dot number" (e.g. ".1") | |
pub const RESERVED_KEYWORDS: &[&str] = &[ | |
"catalog", "config", "font", "health", "help", "index", "manifest", "refresh", "reload", | |
"sprite", "status", | |
]; |
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.
if we start handling
/metrics
, we would have to declare this as a reserved keyword (we might be missing docs in a martin book for this). One workaround perhaps would be to add everything "magical" to the/_/metrics
style prefixes because_
is not a valid source ID i think.martin/martin/src/srv/server.rs
Lines 32 to 37 in a2acd17
/// List of keywords that cannot be used as source IDs. Some of these are reserved for future use. /// Reserved keywords must never end in a "dot number" (e.g. ".1") pub const RESERVED_KEYWORDS: &[&str] = &[ "catalog", "config", "font", "health", "help", "index", "manifest", "refresh", "reload", "sprite", "status", ];
Right, as you wish, I'm fine with both solutions (the reserved keyword and the /_/metrics workaround). We just have to keep in mind that /metrics
is the standard path and the default, so the second solution would mean a little bit more config on the scraper side.
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.
good point about conventions (actually, didn't we talk about it elsewhere?), i'm ok to add /metrics
to reserved then
I don't really what you are saying, the issue is with the variance of |
Are you sure its a big deal to collapse |
yes, you right, for now, let's not worry about this issue, will deal with that later. |
@lefuturiste i added metrics to reserved in #802 , let me know if you need any help with this PR |
@lefuturiste any updates? thx |
Hi! |
while i'm awaiting for a response from the actix-web-prom developer I will explore the instrumentation of postgresql connexion. |
The PR at nlopes/actix-web-prom#73 was merged today. So we can go forward from there. |
exciting, thanks, let me know if you need any help |
For a starter, we can only include the basics metrics from the actix web prom crate.
as discussed in #773