Skip to content

Capture finatra code function name #13939

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

Merged
merged 6 commits into from
Jun 9, 2025
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented May 28, 2025

Based on the discussion in #13860 with new code attribute semantic conventions we need to capture the class of controller method and the method's name.
cc @SylvainJuge

@laurit laurit requested a review from a team as a code owner May 28, 2025 15:59
Class<?> callbackClass = getCallbackClass(route);
// We expect callback to be an inner class of the controller class. If it is not we are not
// going to record it at all.
if (callbackClass != null && callbackClass.getName().startsWith(controllerClass.getName())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is that if callback isn't a nested class in the controller class it could be some kind of generic wrapper function that won't help in locating the actual controller method. Idk maybe that would also be fine and this check should be removed. cc @trask

Copy link
Member

Choose a reason for hiding this comment

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

Idk maybe that would also be fine and this check should be removed.

👍

@laurit laurit mentioned this pull request May 29, 2025
@SylvainJuge
Copy link
Contributor

Thanks for taking the time to implement this @laurit. This looks fine to me but I'm definitely outside of my expertise area so I can't really review it. I'll wait for this to be merged before continuing work on #13860.

@laurit
Copy link
Contributor Author

laurit commented Jun 9, 2025

Thanks for taking the time to implement this @laurit. This looks fine to me but I'm definitely outside of my expertise area so I can't really review it. I'll wait for this to be merged before continuing work on #13860.

Then it could turn into a long wait, unless you can convince someone else from @open-telemetry/java-instrumentation-approvers to do the review.

@SylvainJuge
Copy link
Contributor

Thanks for taking the time to implement this @laurit. This looks fine to me but I'm definitely outside of my expertise area so I can't really review it. I'll wait for this to be merged before continuing work on #13860.

Then it could turn into a long wait, unless you can convince someone else from @open-telemetry/java-instrumentation-approvers to do the review.

Sorry if it wasn't clear, what I meant by that is that I don't know Finatra enough to know if this is enough to cover all possible use-cases.

However, the changes themselves look quite straightforward and I don't see any risk involved by doing them. If needed we can improve that later, so in short it looks good to my non-Finatra expert perspective and I'll approve this PR.

…emetry/javaagent/instrumentation/finatra/FinatraSingletons.java

Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
@trask trask merged commit fcebab2 into open-telemetry:main Jun 9, 2025
88 checks passed
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