poem icon indicating copy to clipboard operation
poem copied to clipboard

Abstract HTTP Path Pattern in Metrics

Open kolloch opened this issue 2 years ago • 1 comments

Description of the feature

For the request metrics, I see two labels that roughly do what I want: http.url and http.path_pattern.

image

http.url is to specific and includes the concrete URL including IDs "resolved" in the pattern. I cannot use it, since it will increase the cardinality of the metric drastically (and therefore increase our bills too much). Basically, a separate time series is stored for all label combinations in most metric systems that I know.

The http.path_pattern sounds like what I want but only includes the route prefix at the point where I mounted the middleware, not of the nested route.

The corresponding watched/subscriber routes are mounting like this:

Route::new().nest(
                    opt.public_base_url.path(), // = /private/nx-release/control/
                    Route::new()
                        .nest("/api", api_service)
                        .at("badges/subscriber/:subscription_id", get(subscriber_badge))
                        .at("badges/watched/:git_ref_id", get(watched_badge))
                        .at("request_dump", post(request_dump))
                        .nest("/", ui)
                        .data(opt.clone())
                        // ... more data(..) ...
                        .data(callback_server.clone())
                        .with(poem::middleware::Tracing)
                        .with(middleware::AddRequestId)
                        .with(OpenTelemetryMetrics::new()) // <- adding the instrumentation to capture all routes

I'd love /private/nx-release/control/badges/subscriber/:subscription_id as the http_pattern (or another label).

Existing Code

This is were the http.path_pattern is added.

https://github.com/poem-web/poem/blob/master/poem/src/middleware/opentelemetry_metrics.rs#L82-L85

        if let Some(path_pattern) = req.data::<PathPattern>() {
            const HTTP_PATH_PATTERN: Key = Key::from_static_str("http.path_pattern");
            labels.push(HTTP_PATH_PATTERN.string(path_pattern.0.to_string()));
        }

PathPattern is a simple string container:

#[derive(Debug)]
#[allow(unreachable_pub)]
pub struct PathPattern(pub Arc<str>);

It is set here:

https://github.com/poem-web/poem/blob/4692e5bb6b3422f05cf9df6a7679ea1c1c005ea9/poem/src/route/router.rs#L319-L328

                match (req.data::<PathPattern>(), req.data::<PathPrefix>()) {
                    (Some(parent), Some(prefix)) => req.set_data(PathPattern(
                        format!("{}{}", parent.0, &pattern[prefix.0..]).into(),
                    )),
                    (None, Some(prefix)) => req.set_data(PathPattern(pattern[prefix.0..].into())),
                    (None, None) => req.set_data(PathPattern(pattern)),
                    (Some(parent), None) => {
                        req.set_data(PathPattern(format!("{}{}", parent.0, pattern).into()))
                    }
                }

Weirdly, there is test code that pretty much asserts the behavior that I want:

https://github.com/poem-web/poem/blob/4692e5bb6b3422f05cf9df6a7679ea1c1c005ea9/poem/src/route/router.rs#L499-L563

    #[tokio::test]
    async fn path_pattern() {
        let app = Route::new()
            .at(
                "/a/:id",
                make_sync(|req| req.data::<PathPattern>().unwrap().0.to_string()),
            )
            .nest(
                "/nest",
                Route::new().at(
                    "/c/:id",
                    make_sync(|req| req.data::<PathPattern>().unwrap().0.to_string()),
                ),
            )
            .nest(
                "/nest1",
                Route::new().nest(
                    "/nest2",
                    Route::new().at(
                        "/:id",
                        make_sync(|req| req.data::<PathPattern>().unwrap().0.to_string()),
                    ),
                ),
            )
            .nest_no_strip(
                "/nest_no_strip",
                Route::new().at(
                    "/nest_no_strip/d/:id",
                    make_sync(|req| req.data::<PathPattern>().unwrap().0.to_string()),
                ),
            )
            .nest_no_strip(
                "/nest_no_strip1",
                Route::new().nest_no_strip(
                    "/nest_no_strip1/nest_no_strip2",
                    Route::new().at(
                        "/nest_no_strip1/nest_no_strip2/:id",
                        make_sync(|req| req.data::<PathPattern>().unwrap().0.to_string()),
                    ),
                ),
            );

        let cli = TestClient::new(app);

        cli.get("/a/10").send().await.assert_text("/a/:id").await;
        cli.get("/nest/c/10")
            .send()
            .await
            .assert_text("/nest/c/:id")
            .await;
        cli.get("/nest_no_strip/d/99")
            .send()
            .await
            .assert_text("/nest_no_strip/d/:id")
            .await;
        cli.get("/nest1/nest2/10")
            .send()
            .await
            .assert_text("/nest1/nest2/:id")
            .await;
        cli.get("/nest_no_strip1/nest_no_strip2/10")
            .send()
            .await
            .assert_text("/nest_no_strip1/nest_no_strip2/:id")
            .await;
    }

I wonder what goes wrong in my case?

kolloch avatar Dec 21 '22 09:12 kolloch

Ah, I assume that the middleware just "takes" the PathPattern at the level it is added.

The test gets the PathPattern inside the the http call itself so it has all the nesting. This is different from what the middleware does if you "mount" it globally.

Can you somehow add it nested to all routes without adding it manually to all leaf routes?

kolloch avatar Dec 21 '22 09:12 kolloch