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 priorities to resource names #3085

Merged
merged 4 commits into from
Oct 7, 2021
Merged

Conversation

devinsba
Copy link
Contributor

Allows for resource name "types" to take precedence without having to worry about write order

Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

Makes sense. Would be great to remove the getResourceNamePriority() if it's not used anywhere.

@devinsba devinsba force-pushed the bds/path-matcher-highest-precedent branch 5 times, most recently from bb1bacc to 031bdd2 Compare September 22, 2021 16:36
@devinsba devinsba force-pushed the bds/path-matcher-highest-precedent branch 9 times, most recently from 2799a8f to f905b64 Compare October 4, 2021 20:45
@@ -151,6 +167,7 @@ class VertxHttpServerForkedTest extends HttpServerTest<Vertx> {
}
}

@Ignore("Route matching doesn't work with a handler before the controller")
Copy link
Contributor Author

@devinsba devinsba Oct 4, 2021

Choose a reason for hiding this comment

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

This is a problem that seems to have already existed. It's going to require a large rework of the instrumentation in order to fix it. Essentially what happens is that every handler added to the router is instrumented, including things like auth handlers, which will usually have a mapping like /* or null but the way we have the instrumentation written only the first handler will evaluate the path as a route. In the case of this test the path is null because of router.route().handler(VertxChainingTestServer::firstHandler);

@devinsba devinsba force-pushed the bds/path-matcher-highest-precedent branch from f905b64 to db76665 Compare October 4, 2021 23:18
@devinsba devinsba changed the title WIP - Add priorities to resource names Add priorities to resource names Oct 4, 2021
@devinsba devinsba marked this pull request as ready for review October 4, 2021 23:39
@devinsba devinsba requested a review from a team as a code owner October 4, 2021 23:39
@devinsba devinsba force-pushed the bds/path-matcher-highest-precedent branch from db76665 to e9e1ba4 Compare October 5, 2021 14:44
@devinsba devinsba added the inst: others All other instrumentations label Oct 7, 2021
@devinsba devinsba merged commit 7780882 into master Oct 7, 2021
@devinsba devinsba deleted the bds/path-matcher-highest-precedent branch October 7, 2021 17:25
@github-actions github-actions bot added this to the 0.89.0 milestone Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: others All other instrumentations tag: breaking change Breaking changes type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants