Navigation Menu

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

Migrate trace_methods to indy plugin #2094

Merged
merged 4 commits into from Sep 1, 2021

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Aug 30, 2021

What does this PR do?

Relates to #1337

Checklist

@felixbarny felixbarny mentioned this pull request Aug 30, 2021
37 tasks
@apmmachine
Copy link
Collaborator

apmmachine commented Aug 30, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-31T18:39:23.055+0000

  • Duration: 55 min 15 sec

  • Commit: b71886a

Test stats 🧪

Test Results
Failed 0
Passed 2395
Skipped 19
Total 2414

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2395
Skipped 19
Total 2414

@@ -32,6 +33,7 @@
import static co.elastic.apm.agent.matcher.AnnotationMatcher.annotationMatcher;
import static co.elastic.apm.agent.matcher.WildcardMatcher.caseSensitiveMatcher;

@GlobalState
Copy link
Member

Choose a reason for hiding this comment

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

[question] If I understand it correctly, we only need global state here as it relates to configuration like MethodMatcherValueConverter, and thus needs to be loaded in the main agent classloader and not in the plugin classloader, is that correct ? If so, maybe a small comment could make that slightly more obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Agree on adding an explanatory comment.


public static class TraceMethodAdvice {

private static final ElasticApmTracer tracer = GlobalTracer.requireTracerImpl();
Copy link
Member

Choose a reason for hiding this comment

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

[minor] moving tracer init to the start of the static block makes the dependency to retrieve configuration obvious.

@felixbarny felixbarny merged commit 3306620 into elastic:master Sep 1, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Sep 1, 2021
@felixbarny felixbarny deleted the trace-methods-indy branch September 1, 2021 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants