Skip to content
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

stats: Extract route names with forward-slash #33734

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Conversation

kb000
Copy link
Contributor

@kb000 kb000 commented Apr 22, 2024

Commit Message: Extract route names with forward-slash.
Additional Description: RDS protocol allows almost any route name, and stats are lightly sanitized before emitting to statsd and prometheus endpoints. This change slightly relaxes the capture group for RDS route names in stats, instead of opening the floodgates to everything, because YAGNI.
Risk Level: Low
Testing: Updated unit test
Docs Changes: None
Release Notes: N/A
Platform Specific Features: None
Fixes: #32976

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #33734 was opened by kb000.

see: more, trace.

@kb000 kb000 changed the title Extract route names with forward-slash from stats stats: Extract route names with forward-slash Apr 22, 2024
@kb000 kb000 marked this pull request as ready for review April 22, 2024 18:31
@jmarantz
Copy link
Contributor

/wait for CI

@kb000
Copy link
Contributor Author

kb000 commented Apr 23, 2024

/retest

@kb000
Copy link
Contributor Author

kb000 commented Apr 24, 2024

/ptal @jmarantz

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm modulo the usual concern that this might be a breaking change for someone. But I couldn't think of how it would break anything, so I'm approving.

@ggreenway wdyt?

@jmarantz
Copy link
Contributor

jmarantz commented Apr 24, 2024

Oh...one thing: can you take a quick look at the docs for route stats and see if they need to be updated?

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jmarantz
Copy link
Contributor

@kb100 before I merge can you just check to see if we should update doc for this syntax change?

@kb000
Copy link
Contributor Author

kb000 commented Apr 25, 2024

Oh...one thing: can you take a quick look at the docs for route stats and see if they need to be updated?

Oh it looks like the regex needs to be a little more relaxed. Docs say ":" is allowed (and replaced), and it's not.
https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/rds#statistics

@kb000
Copy link
Contributor Author

kb000 commented Apr 25, 2024

Oh...one thing: can you take a quick look at the docs for route stats and see if they need to be updated?

Oh it looks like the regex needs to be a little more relaxed. Docs say ":" is allowed (and replaced), and it's not. https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/rds#statistics

Nevermind, I misread it. The ":" never makes it into the stat name. It's sanitized out by sanitizeStatsName well before the tag extraction code.

@kb000
Copy link
Contributor Author

kb000 commented Apr 25, 2024

@jmarantz no doc updates needed 👍
No restrictions on the route name in RDS were documented before (anything goes, as far as I can tell), so there's nothing to change there.
The minimal documentation on route name in RDS stats don't mention allowing, disallowing, or special handling for anything but ":", so I'd prefer to leave it vague.

@jmarantz jmarantz merged commit d471d48 into envoyproxy:main Apr 30, 2024
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats: RDS_ROUTE_CONFIG tag extraction
3 participants