pekko: Improve PekkoRouteHolder#16390
Conversation
- Give each Directive its own PekkoRouteHolder to avoid concurrent access. Only when there's a route match are the PekkoRouteHolders in that call stack combined. - Remove need for state queue - remove query params from tapir http.route, as required by otel convention - support Tapir's internal route concatenation feature
Co-authored-by: Trask Stalnaker <[email protected]>
- Fix Nullable import - Add volatile to fields accessed in different threads - Move constructor to top of PekkoRouteHolder - Use '.flatMap(routeHolder -> Optional.ofNullable(routeHolder.route()))' to make clear that `routeHolder.route()` can return `null`.
|
hey @samwright, fyi both of the above reviews from me were really from AI (based on proposal in #17889), sorry I thought I had mentioned that above, but apparently not |
- LinkedList -> ArrayList + ArrayDeque - no unnecessary onThrowable in advice
That's cool - I had already guessed :-) |
| PekkoRouteHolder routeHolder = PekkoRouteHolder.create(); | ||
| Context context = | ||
| instrumenter() | ||
| .start(parentContext, request) | ||
| .with(routeHolder) | ||
| .with(new PekkoFallbackRouteHolder()); |
There was a problem hiding this comment.
does it make sense to store the PekkoFallbackRouteHolder inside the PekkoRouteHolder, and not add the PekkoFallbackRouteHolder to the context?
There was a problem hiding this comment.
The PekkoRouteHolder is attached to the HttpRequest, and each time PekkoRouteWrapper is applied another PekkoRouteHolder instance is created and attached to the HttpRequest. So adding anything to one of the child routeHolders won't affect the original routeHolder. We could find the original by calling routeHolder.parent until there is no parent. Would that be preferable to having this extra Context item?
There was a problem hiding this comment.
that sounds good, it's nice to avoid extra context items, as long as the alternative isn't complex
| public void setOverride(PekkoRouteHolder override) { | ||
| if (override.override != null) { | ||
| // The given override itself has an override, so just use that one instead | ||
| this.override = override.override; |
There was a problem hiding this comment.
can there be multiple layers of overrides?
There was a problem hiding this comment.
No, this branch ensures that if the given routeHolder A has an override B, this.override is set to B. Since every holder starts with no override, this ensures the max depth of overide.overide.overide... is 1.
| public String route() { | ||
| return lastWasMatched ? route.toString() : null; | ||
| public boolean hasRoute() { | ||
| return override != null || lastWasMatched; |
There was a problem hiding this comment.
similar question whether overrides can have overrides (and if that affects calculation here)?
There was a problem hiding this comment.
see answer below, but no it's not possible.
The previous prompt's guidelines were under-specified, so the model ran on intuition: when an approver had left review questions and the author hadn't replied (open-telemetry#16390), it picked 'approver' instead of 'author'. Restate the rules as an ordered first-match decision tree (external > author-owes-response > approver-default), with explicit edge cases: an approver *commit* does not put the ball in the author's court, and an approver comment linking to an upstream issue should bucket as external.
…er-immutable3 # Conflicts: # instrumentation/pekko/pekko-http-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/pekkohttp/v1_0/server/tapir/TapirPathInstrumentation.java
- Don't precalculate string length - use `_` for ignored variables in test
- Remove PekkoFallbackRouteHolder
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
Thanks! One optional suggestion if it makes sense to you.
| boolean shouldAddFinalWildcard = lastUnmatchedPath != null && !lastUnmatchedPath.isEmpty(); | ||
| Deque<String> routePaths = new ArrayDeque<>(); | ||
|
|
||
| for (PekkoRouteHolder routeHolder = this; | ||
| routeHolder != null; | ||
| routeHolder = routeHolder.parent) { | ||
|
|
||
| for (int i = routeHolder.paths.size() - 1; i >= 0; i--) { | ||
| routePaths.addFirst(routeHolder.paths.get(i)); | ||
| } | ||
| } | ||
| if (shouldAddFinalWildcard) { | ||
| routePaths.addLast("*"); | ||
| } | ||
| return String.join("", routePaths); | ||
| } |
There was a problem hiding this comment.
Optional: a parent-recursion appendRoute helper is a bit shorter and avoids the intermediate Deque plus the reverse-iteration over each holder's paths.
| boolean shouldAddFinalWildcard = lastUnmatchedPath != null && !lastUnmatchedPath.isEmpty(); | |
| Deque<String> routePaths = new ArrayDeque<>(); | |
| for (PekkoRouteHolder routeHolder = this; | |
| routeHolder != null; | |
| routeHolder = routeHolder.parent) { | |
| for (int i = routeHolder.paths.size() - 1; i >= 0; i--) { | |
| routePaths.addFirst(routeHolder.paths.get(i)); | |
| } | |
| } | |
| if (shouldAddFinalWildcard) { | |
| routePaths.addLast("*"); | |
| } | |
| return String.join("", routePaths); | |
| } | |
| StringBuilder sb = new StringBuilder(); | |
| appendRoute(sb); | |
| if (lastUnmatchedPath != null && !lastUnmatchedPath.isEmpty()) { | |
| sb.append('*'); | |
| } | |
| return sb.toString(); | |
| } | |
| private void appendRoute(StringBuilder sb) { | |
| if (parent != null) { | |
| parent.appendRoute(sb); | |
| } | |
| for (String path : paths) { | |
| sb.append(path); | |
| } | |
| } |
There was a problem hiding this comment.
I like the brevity, but that Deque is being replaced by frames in the stack, which could be a problem for very long/complex PathMatchers.
The reverse-iteration is cheap (operating on an ArrayList) so I don't think there's a performance improvement here (happy to be proven wrong though). I could replace Strings.join with a StringBuilder, if that's preferred?
There was a problem hiding this comment.
makes sense, let's merge as-is, thanks!
|
Thank you for your contribution @samwright! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
NullPointerExceptionandArrayIndexOutOfBoundsExceptionwith pekko-http #15681. Only when there's a route match are the PekkoRouteHolders in that call stack combined.