-
Notifications
You must be signed in to change notification settings - Fork 979
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
Conversation
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())) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
👍
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. |
...gent/src/main/java/io/opentelemetry/javaagent/instrumentation/finatra/FinatraSingletons.java
Outdated
Show resolved
Hide resolved
…emetry/javaagent/instrumentation/finatra/FinatraSingletons.java Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
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