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

heuristic: make wildcard character configurable #1418

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

marevers
Copy link
Contributor

@marevers marevers commented Nov 28, 2024

Closes #1410

Changes the route transform heuristic mode to replace route/path components (gibberish and numeric ids) with a configurable single character. * remains the default. This improves handling of label selectors, as * is a regex token and can interfere with regex handling in Prometheus/Grafana. By providing the option of changing the replacement character, this can be avoided if required or wanted.

@marevers
Copy link
Contributor Author

I am not sure if in light of this change the wildCard constant should also be changed to /##.

const wildCard = "/**"

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.00%. Comparing base (85b6cdd) to head (f65ce4b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
+ Coverage   76.86%   81.00%   +4.14%     
==========================================
  Files         149      149              
  Lines       15252    15255       +3     
==========================================
+ Hits        11723    12357     +634     
+ Misses       2911     2294     -617     
+ Partials      618      604      -14     
Flag Coverage Δ
integration-test 59.85% <50.00%> (+0.05%) ⬆️
k8s-integration-test 59.51% <43.75%> (?)
oats-test 33.91% <25.00%> (-0.01%) ⬇️
unittests 51.98% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marevers marevers marked this pull request as draft December 9, 2024 10:01
@marevers marevers changed the title heuristic: replace with # rather than with * heuristic: make wildcard character configurable Dec 9, 2024
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @marevers again! This looks good to me, I just left one comment around supplying a char to the replace function rather than a string. If we can make it work with char everywhere that would be even better, I just don't know if it's possible.

@@ -45,7 +45,7 @@ func InitAutoClassifier() error {
}

//nolint:cyclop
func ClusterPath(path string) string {
func ClusterPath(path string, replacement string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit here, can you please change the function second argument to be a char? That way we don't need to dereference the string multiple times.

Copy link
Contributor Author

@marevers marevers Dec 11, 2024

Choose a reason for hiding this comment

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

I ended up solving this a little differently with my commit 819296b. If I were to use a rune parameter, it would necessitate another type casting to byte as within the ClusterPath function, a byte is required. So I thought it better to use a byte parameter instead and move the deferencing of the string to the ClusterPath call within classifyFromPath. I hope you're okay with that. Tests were adjusted to account for this change.

pkg/transform/routes.go Show resolved Hide resolved
@marevers marevers force-pushed the heuristic-pound-sign branch from 0507f11 to 819296b Compare December 11, 2024 08:31
@marevers marevers marked this pull request as ready for review December 11, 2024 08:42
@grcevski
Copy link
Contributor

Looks great @marevers! I think we just need to make the docs linter happy and I'm good to merge this. Great work as usual!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using routes.unmatched: heuristic interferes with Prometheus/Grafana regex interpretation
3 participants