WIP: feat(metrics): add actix-web metrics exporter

[lefuturiste]

For a starter, we can only include the basics metrics from the actix web prom crate.

as discussed in #773

[nyurik]

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?

[nyurik]

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",
	];

[lefuturiste]

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.

[nyurik]

good point about conventions (actually, didn't we talk about it elsewhere?), i'm ok to add /metrics to reserved then

[lefuturiste]

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?

I don't really what you are saying, the issue is with the variance of /srcA,srcB, could also be /srcB,srcA (I suppose)
So probably the best thing to do is to have a magical callback that is defined on the maplibre side, that take the request parameter and request path and return the path for metrics. So the business logic stay downstream and not defined in the lib.
But, I don't recall that we settled on a solution yet.

[nyurik]

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?

I don't really what you are saying, the issue is with the variance of /srcA,srcB, could also be /srcB,srcA (I suppose) So probably the best thing to do is to have a magical callback that is defined on the maplibre side, that take the request parameter and request path and return the path for metrics. So the business logic stay downstream and not defined in the lib. But, I don't recall that we settled on a solution yet.

Are you sure its a big deal to collapse /srcA,srcB and /srcB,srcA into one? I wouldn't worry too much about this tbh... Worst case - can later add some sort of a collapser, e.g. /srcA+ would mean everything with srcA as the first layer, plus any number of other layers mixed in. The collapser could even be configurable to how many sources to keep, e.g. 0 -- /+ means all multi-sourced requests, or 2 would create /srcA,srcB and /srcA,srcB+ metrics

[lefuturiste]

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?

I don't really what you are saying, the issue is with the variance of /srcA,srcB, could also be /srcB,srcA (I suppose) So probably the best thing to do is to have a magical callback that is defined on the maplibre side, that take the request parameter and request path and return the path for metrics. So the business logic stay downstream and not defined in the lib. But, I don't recall that we settled on a solution yet.

Are you sure its a big deal to collapse /srcA,srcB and /srcB,srcA into one? I wouldn't worry too much about this tbh... Worst case - can later add some sort of a collapser, e.g. /srcA+ would mean everything with srcA as the first layer, plus any number of other layers mixed in. The collapser could even be configurable to how many sources to keep, e.g. 0 -- /+ means all multi-sourced requests, or 2 would create /srcA,srcB and /srcA,srcB+ metrics

yes, you right, for now, let's not worry about this issue, will deal with that later.

[nyurik]

@lefuturiste i added metrics to reserved in #802 , let me know if you need any help with this PR

[nyurik]

@lefuturiste any updates? thx

[lefuturiste]

@lefuturiste any updates? thx

Hi!
I've updated upstream PR for this, because I really want to have path cardinality (idk really why an obsession may be 😄 ), nlopes/actix-web-prom#73
I awaiting response from actix-web-prom dev.

[lefuturiste]

while i'm awaiting for a response from the actix-web-prom developer I will explore the instrumentation of postgresql connexion.

[lefuturiste]

The PR at nlopes/actix-web-prom#73 was merged today. So we can go forward from there.

[nyurik]

exciting, thanks, let me know if you need any help