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

fix bug: get return type in a wrong way for method instrumentation #6118

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

darcydai
Copy link
Contributor

@darcydai darcydai commented May 29, 2022

After testing, @Advice.Origin("#r") can only insert a string type parameter from the constant pool.
@Advice.Origin("#r") Class<?> returnType: returnType is current class for method

@darcydai darcydai requested a review from a team May 29, 2022 15:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 29, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: darcydai / name: Darcy (6ff3d4d)

@laurit
Copy link
Contributor

laurit commented May 30, 2022

@darcydai could you add a test for this?

@darcydai
Copy link
Contributor Author

@darcydai could you add a test for this?

yes, later

@darcydai
Copy link
Contributor Author

@darcydai could you add a test for this?

it is hard to approve @Advice.Origin("#r") Class<?> returnType is the wrong way to get the return type by a test case,because this is bytebuddy feature.
I wrote a test case that use @Advice.Origin("#r") Class<?> returnType can not pass.

@laurit
Copy link
Contributor

laurit commented May 31, 2022

@darcydai I created a pull request darcydai#1 for your pull request. I moved the new test into a separate method and instead of figuring out from span length checked directly whether span was still running after the method completed. WDYT?

@darcydai
Copy link
Contributor Author

WDYT

wow, that is more graceful than mine

optimize test case on method instrumentation
@darcydai
Copy link
Contributor Author

@darcydai I created a pull request darcydai#1 for your pull request. I moved the new test into a separate method and instead of figuring out from span length checked directly whether span was still running after the method completed. WDYT?

I have merged your pull request

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx!

@trask trask merged commit 925f619 into open-telemetry:main Jun 1, 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.

4 participants