Skip to content

Feature support jakartaee9#1912

Merged
jackshirazi merged 46 commits intoelastic:masterfrom
videnkz:feature-support-jakartaee9
Oct 18, 2021
Merged

Feature support jakartaee9#1912
jackshirazi merged 46 commits intoelastic:masterfrom
videnkz:feature-support-jakartaee9

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Jul 12, 2021 •
edited by zube bot
Loading

What does this PR do?

Implements the servlet part for #1543

Checklist

  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • TODO - fix error span creation for jetty 11
videnkz added 7 commits May 23, 2021 02:11
@videnkz
splitting servlet plugin
8f4f919
@videnkz
local
eeda836
@videnkz
Merge remote-tracking branch 'upstream/master' into feature-support-j…
72585b1 
…akartaee9

# Conflicts:
#	apm-agent-plugins/apm-servlet-plugin/pom.xml
#	apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/CommonServletInstrumentation.java
#	apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/RequestStreamRecordingInstrumentation.java
#	apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletApiAdvice.java
#	apm-agent-plugins/apm-servlet-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.sdk.ElasticApmInstrumentation
@videnkz
implement jakarta async instrumentation
a759b9e
@videnkz
local
5b89c9d
@videnkz
differntiate ServletApiADvice
213f62c
@videnkz
added apm servlet jakarta test
142dae3
@ghost
Copy link

ghost commented Jul 12, 2021 •
edited by ghost
Loading

💚 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-10-18T20:48:29.240+0000

  • Duration: 65 min 10 sec

  • Commit: ff7b1df

Test stats 🧪

Test Results
Failed 0
Passed 2474
Skipped 19
Total 2493

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run compatibility tests : Run the JDK Compatibility test.

  • run integration tests : Run the APM-ITs.
@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 18, 2021

@kananindzya thanks a lot!! It will probably take us a bit until we get to review it, but we will get to this.
@eyalkoren eyalkoren mentioned this pull request Jul 18, 2021
Open
8 tasks
@videnkz videnkz marked this pull request as draft July 18, 2021 14:00
@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 22, 2021 •
edited
Loading

Wow!! Great progress @kananindzya !! 👏 👏 👏
What else is still missing? Is the Jetty 11 issue already handled?
@videnkz
Copy link
Contributor Author

videnkz commented Jul 22, 2021

Is the Jetty 11 issue already handled?

Remained a problem with jetty 11, creating a span for the ERROR DispatcherType.
@videnkz videnkz marked this pull request as ready for review July 29, 2021 18:24
@videnkz
Copy link
Contributor Author

videnkz commented Sep 14, 2021

@kananindzya since you merged #2101 , the default instrumentation implementation now looks for an internal AdviceClass. Once this PR was merged, you cannot use your instrumentation classes as advice classes (classes that contain the Byte Buddy @Advice.OnMethodEnter/@Advice.OnMethodExit annotations) anymore. Instead, you must extract the advice code into a separate class that is either called AdviceClass, or called differently and override ElasticApmInstrumentation#getAdviceClassName() accordingly.

The tests fail now because this is not applied to your new JakartaServletVersionInstrumentation instrumentations (and other new instrumentations if there are such).

Take a look at #2101 for examples.

fixed
@videnkz
Copy link
Contributor Author

videnkz commented Sep 14, 2021 •
edited
Loading

Closes #1523
Closes #1335

@jackshirazi I just started tests, let's see how long it takes with this PR. A lot of integration tests were added, so it may take considerably longer than it did previously. If that's the case, we should consider reducing the number of tested versions, maybe only restrict to LTS and latest versions.

@jackshirazi , @eyalkoren please review . I left the LTS versions
@videnkz videnkz requested a review from jackshirazi September 14, 2021 17:50
@jackshirazi
Copy link
Contributor

jackshirazi commented Sep 30, 2021

/test
@jackshirazi
Copy link
Contributor

jackshirazi commented Sep 30, 2021

/test
@jackshirazi
Copy link
Contributor

jackshirazi commented Oct 6, 2021

/test
@jackshirazi
Copy link
Contributor

jackshirazi commented Oct 12, 2021

/test
@AlexanderWert AlexanderWert modified the milestones: 7.16, 8.0 Oct 18, 2021
@jackshirazi jackshirazi merged commit b44fc14 into elastic:master Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-java community Issues and PRs created by the community

4 participants

@videnkz @eyalkoren @jackshirazi @AlexanderWert