fix for armeria 1.40#11827
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf18f6c445
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| named("close") | ||
| .and(isMethod()) | ||
| .and(takesArgument(0, named("io.grpc.Status"))) | ||
| .and(takesArgument(1, named("io.grpc.Metadata"))), |
There was a problem hiding this comment.
On Armeria 1.40 this matcher also instruments close(Status, Metadata, Throwable), but that method can now return immediately when this.closeCause != closeCause to ignore a transport close while an exception-handler close is still pending. Because CloseObserver removes the span on method enter and finishes it on exit, an ignored close can consume and finish the client span before the later real close runs, so calls that hit listener/deserialization errors or async exception handling lose the final gRPC status/error metadata. The advice needs to avoid removing/finishing the span until the Armeria close actually proceeds.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: fdbd9a1 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| @Advice.OnMethodEnter | ||
| public static AgentScope before(@Advice.This ClientCall<?, ?> call) { | ||
| // could create a message span here for the request | ||
| AgentSpan span = InstrumentationContext.get(ClientCall.class, AgentSpan.class).get(call); |
There was a problem hiding this comment.
compared to the "original" advice, we do a get here instead of a remove call (because we don't know yet if we're going to need to remove the span yet)
| if (null != scope) { | ||
| if (closed) { | ||
| AgentSpan span = | ||
| InstrumentationContext.get(ClientCall.class, AgentSpan.class).remove(call); |
There was a problem hiding this comment.
the span is removed from the context here instead, once we can read the value of "closed"
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What Does This Do
armeria 1.40 adds an extra argument to instrumented (private) method
ArmeriaClientCall.close, now takes a throwable as third arg.Fixed by relaxing the constraint on
takesArgument(2), replaced by a constraint on the type of the first 2 to keep it narrow enough.Motivation
latest dep tests failed
Additional Notes
not sure if we should use that Throwable to set as error on the current span ?
Just changing the selector is enough to get the tests to pass so I think we should merge this as a quick fix, and reflect on this maybe later.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]