feat: configurable path parameters cardinality #73

lefuturiste · 2023-08-07T15:36:30Z

the idea for this MR was originally discussed in #72

A change that allow developer to change the cardinality of a given param name for a specific route.

To define a config that goes down the line, we need to use actix extensions_mut() method. The external/user developer need to define a middleware that will pass some data down the middleware stack.

Example when defining a route:

    web::resource("/{source_ids}/{z}/{x}/{y}")
        .wrap_fn(|req, srv| {
            req.extensions_mut().insert::<MetricsConfig>(
                MetricsConfig { cardinality_keep_params: vec!["source_ids".to_string()] }
            );
            srv.call(req)
        })
        .route(web::get().to(get_tile_handler))

Feel free to comment or help.

lefuturiste · 2023-08-11T17:07:42Z

@nlopes You may take a look at this

nlopes · 2023-09-06T10:04:34Z

Thanks for sending over. I'll try to take a look sometime this week.

lefuturiste · 2023-10-05T09:16:19Z

Thanks for sending over. I'll try to take a look sometime this week.

Hello, I'm kindly asking if you have some update on this, thanks! 😄

nlopes · 2023-10-08T18:15:46Z

Apologies for taking so long. Can you modify your example please? As it stands I don't think it actually exemplifies your change. Or am I misunderstanding it?

lefuturiste · 2023-10-08T19:37:48Z

Apologies for taking so long. Can you modify your example please? As it stands I don't think it actually exemplifies your change. Or am I misunderstanding it?

Yes you are correct, I will update the example.

the main purpose for me is to have an example on which I can test with configurable params cardinality feature.

Allow developer to externally set which on route params they want to keep in the metrics (as metrics label cardinality) on a particular route.

lefuturiste · 2023-10-09T20:54:11Z

In theory I should have rebased this fork, and now I added an example. I also need to fix the code so that 404 are not counting as new labels values.

TODO:

clarify example
add unit test
fix 404 behavior
add test for 404 behavior

lefuturiste · 2023-10-10T09:45:09Z

@nlopes can you take a look at this? It should be better now

lefuturiste · 2023-11-17T17:41:17Z

Hello @nlopes sorry to ask you again, what can be done to get this forward? I know that maintaining opensource software is a burden 😢 .

nlopes

Almost there. Apologies, I've forgotten about this.

nlopes · 2023-11-17T17:54:04Z

README.md

+### Configurable routes pattern cardinality
+
+Let's say you have on your app a route to fetch posts by language and by slug `GET /posts/{language}/{slug}`.
+By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{locale}/{slug}`.


Suggested change

By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{locale}/{slug}`.

By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{language}/{slug}`.

nlopes · 2023-11-17T17:55:57Z

src/lib.rs

            || self.exclude_status.contains(&status)
        {
            return;
        }

+        let final_pattern = 
+            if fallback_pattern != pattern && (status == 404 || status == 405) { fallback_pattern }


Can you add a comment here on why this is necessary? I'm pretty sure we'll all forget why at some point.

nlopes · 2023-11-17T20:55:33Z

src/lib.rs

+                match strfmt(&full_pattern, &params) {
+                    Ok(mixed_cardinality_pattern) => mixed_cardinality_pattern,
+                    Err(_) => {
+                        // may be we need to log something to warn that we didn't managed to build the pattern?


Yes, probably useful to bring in the log package.

fix: add warn log when failed to build mixed cardinality pattern docs: typo fix (apply review suggestions)

lefuturiste · 2023-11-18T14:07:36Z

Almost there. Apologies, I've forgotten about this.

Thanks for your review, I've fix something

lefuturiste marked this pull request as ready for review September 6, 2023 09:33

lefuturiste changed the title ~~WIP: configurable for path cardinality~~ feat: configurable for path cardinality Sep 6, 2023

lefuturiste changed the title ~~feat: configurable for path cardinality~~ feat: configurable path params cardinality Sep 6, 2023

lefuturiste changed the title ~~feat: configurable path params cardinality~~ feat: configurable path parameters cardinality Sep 6, 2023

lefuturiste mentioned this pull request Sep 6, 2023

WIP: feat(metrics): add actix-web metrics exporter maplibre/martin#793

Draft

lefuturiste added 3 commits October 9, 2023 17:59

docs: add a examples with path parameters

ec27318

the main purpose for me is to have an example on which I can test with configurable params cardinality feature.

feat: configurable path cardinality

d2fc5ba

Allow developer to externally set which on route params they want to keep in the metrics (as metrics label cardinality) on a particular route.

docs: clarify params cardinality example

d025d19

lefuturiste force-pushed the main branch from ed48028 to d025d19 Compare October 9, 2023 20:51

lefuturiste changed the title ~~feat: configurable path parameters cardinality~~ WIP: feat: configurable path parameters cardinality Oct 9, 2023

lefuturiste added 2 commits October 10, 2023 08:39

tests: configurable path parameters cardinality and fixes

8632645

fix: discard additional variation of params on 404 responses

a05883e

lefuturiste changed the title ~~WIP: feat: configurable path parameters cardinality~~ feat: configurable path parameters cardinality Oct 10, 2023

docs: introduction to configurable params cardinality

1566320

nlopes reviewed Nov 17, 2023

View reviewed changes

refactor: rename pattern as mixed_pattern and add comments

50c01e1

fix: add warn log when failed to build mixed cardinality pattern docs: typo fix (apply review suggestions)

lefuturiste force-pushed the main branch from 0e711bf to 50c01e1 Compare November 18, 2023 10:00

style: apply cargo format

7ca90f9

nlopes merged commit 817fe1e into nlopes:main Dec 1, 2023
3 checks passed

lefuturiste mentioned this pull request Aug 29, 2024

Question: How to avoid WARN actix_web_prom: Cannot build mixed cardinality pattern ? #82

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: configurable path parameters cardinality #73

feat: configurable path parameters cardinality #73

lefuturiste commented Aug 7, 2023 •

edited

Loading

lefuturiste commented Aug 11, 2023

nlopes commented Sep 6, 2023

lefuturiste commented Oct 5, 2023

nlopes commented Oct 8, 2023

lefuturiste commented Oct 8, 2023 •

edited

Loading

lefuturiste commented Oct 9, 2023 •

edited

Loading

lefuturiste commented Oct 10, 2023 •

edited

Loading

lefuturiste commented Nov 17, 2023

nlopes left a comment

nlopes Nov 17, 2023

nlopes Nov 17, 2023

nlopes Nov 17, 2023

lefuturiste commented Nov 18, 2023

	By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{locale}/{slug}`.
	By default, actix-web-prom will provide metrics for the whole route with the label `endpoint` set to the pattern `/posts/{language}/{slug}`.

feat: configurable path parameters cardinality #73

feat: configurable path parameters cardinality #73

Conversation

lefuturiste commented Aug 7, 2023 • edited Loading

lefuturiste commented Aug 11, 2023

nlopes commented Sep 6, 2023

lefuturiste commented Oct 5, 2023

nlopes commented Oct 8, 2023

lefuturiste commented Oct 8, 2023 • edited Loading

lefuturiste commented Oct 9, 2023 • edited Loading

lefuturiste commented Oct 10, 2023 • edited Loading

lefuturiste commented Nov 17, 2023

nlopes left a comment

Choose a reason for hiding this comment

nlopes Nov 17, 2023

Choose a reason for hiding this comment

nlopes Nov 17, 2023

Choose a reason for hiding this comment

nlopes Nov 17, 2023

Choose a reason for hiding this comment

lefuturiste commented Nov 18, 2023

lefuturiste commented Aug 7, 2023 •

edited

Loading

lefuturiste commented Oct 8, 2023 •

edited

Loading

lefuturiste commented Oct 9, 2023 •

edited

Loading

lefuturiste commented Oct 10, 2023 •

edited

Loading