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

record exception in dubbo high version #5892

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

tydhot
Copy link
Member

@tydhot tydhot commented Apr 20, 2022

in dubbo server side, it will not throw exception. instead it will put the exception in Result, so we miss the exception in server side now

@tydhot tydhot requested a review from a team as a code owner April 20, 2022 07:37
@@ -63,7 +63,7 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) {
throw e;
}
if (isSynchronous) {
instrumenter.end(context, request, result, null);
instrumenter.end(context, request, result, result.getException());
Copy link
Contributor

Choose a reason for hiding this comment

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

should async case also use something like throwable != null ? throwable : result.getException()?

Copy link
Member Author

Choose a reason for hiding this comment

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

this change is only for server side. the async case is for the client in dubbo. I check the code in brave too, it only use 'result.getException()' in sync case too.

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Thanks!

@trask trask merged commit 0c1afee into open-telemetry:main Apr 20, 2022
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
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

4 participants