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

Add URL clustering package #447

Closed
wants to merge 3 commits into from
Closed

Add URL clustering package #447

wants to merge 3 commits into from

Conversation

eskirk
Copy link

@eskirk eskirk commented Dec 6, 2023

What this PR does:
This PR aims to add a new package to dskit called clusterurl which will process a string URL and return a generic "page route".

For example, the path "/foo/42/baz" would be replaced with "/foo/*/baz".

Which issue(s) this PR fixes:
This solves issue #privateIssue. Additionally, since both the Frontend Observability and Beyla/Instrumentations teams plan on using this functionality, it is best to put it in a place where it is easily reusable and discoverable.

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Notes
Thanks to @grcevski and the Beyla team for creating this code - I am just packaging it up for them :)

Copy link

@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 for making this part of a generic library, once merged we can start using this common package in Beyla.

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

I'm skeptical that this makes sense to put in dskit. This seems very specific to your application, even if it would happen to be used by multiple projects. It doesn't seem unreasonable to make another repo for this shared dependency between the projects in question.

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Blocking because dskit is isn't meant to be the only way to share dependencies between projects, another repo would make more sense for this particular functionality.

@eskirk
Copy link
Author

eskirk commented Dec 6, 2023

@56quarters got it - that makes sense to me. I was told this would be a good place to put it so I am not super invested in it necessarily being in this repo. going to close this PR in that case 👍🏼

@eskirk eskirk closed this Dec 6, 2023
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.

None yet

3 participants