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

Universal bootdelegation #1259

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Jun 29, 2020

What does this PR do?

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • 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 CHANGELOG.asciidoc
    • 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.
  • This is something else

@apmmachine
Copy link
Contributor

apmmachine commented Jun 29, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1259 updated]

  • Start Time: 2020-06-30T07:23:56.962+0000

  • Duration: 39 min 11 sec

Test stats 🧪

Test Results
Failed 0
Passed 312
Skipped 10
Total 322

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

A PR that removes 3X more than it adds, I like those ❤️ !!
The JBoss and WildFly integration tests would fail without the new plugin?
I would ask @michaelhyatt to test a snapshot of that with Mule 4 and maybe the users that reported the Fuse, Pentaho, Nexus issues as well before merging. Get as much indication as we can that we don't introduce a regression with that.

@felixbarny
Copy link
Member Author

The JBoss and WildFly integration tests would fail without the new plugin?

Yes, and also the Payara test which uses an OSGi container.

Get as much indication as we can that we don't introduce a regression with that.

I was thinking we should do that one we have a release candidate that also contains #1230 so we don't have to ask everyone to test multiple times.

@eyalkoren
Copy link
Contributor

@philippkahr Can you test the relevant artifact from here and see if this works on your Atlassian systems without any special configuration we may have added in the past to deal with OSGi?

@eyalkoren
Copy link
Contributor

I was thinking we should do that one we have a release candidate that also contains #1230 so we don't have to ask everyone to test multiple times.

NP, just asked this once 😊 as this is very OSGi specific. But good idea, once we merge both, let's just keep in mind we want these special tests, specifically Mule which is not a proper OSGi, so would make an interesting case.

@philippkahr
Copy link

@eyalkoren that works just as expected! Right out of the box, no issue.

What I would suggest is to add e.g. jira with a few different versions to your build / end2end testing pipeline.

docker run -v /Users/e_pkah/Desktop/setenv.sh:/opt/atlassian/jira/bin/setenv.sh -v /Users/e_pkah/Desktop/elastic-apm-agent-1.17.1-SNAPSHOT.jar:/opt/atlassian/jira/elastic-apm-agent.jar --name="jira" -d -p 8080:8080 atlassian/jira-software

with that you get a jira container that is ready after a few seconds and you can run a curl against http://localhost:8080 and check if something is broken ;). I would say, as long as it returns something, that is a good sign.

https://gist.github.com/philippkahr/6c12c99028e5dfc08847edb73dc9f4cf here you go for the successful Catalina log. I added the setenv.sh I used, so you can recreate it at any given time.

image

@felixbarny felixbarny merged commit e6737ac into elastic:master Jul 1, 2020
@felixbarny felixbarny deleted the universal-bootdelegation branch July 1, 2020 08:37
@SylvainJuge
Copy link
Member

Also, for the record, this change prevents having such awful error messages with Jboss at server shutdown (Jboss EAP 7.3.0) when using runtime attachment.

java.lang.NoClassDefFoundError: co/elastic/apm/agent/concurrent/JavaConcurrent
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor.execute(EnhancedQueueExecutor.java:763)
   at [email protected]//org.jboss.msc.service.ServiceContainerImpl$ContainerExecutor.execute(ServiceContainerImpl.java:945)
   at [email protected]//org.jboss.msc.service.ServiceControllerImpl.doExecute(ServiceControllerImpl.java:795)
   at [email protected]//org.jboss.msc.service.ServiceControllerImpl$ControllerTask.run(ServiceControllerImpl.java:1569)
   at [email protected]//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1982)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
   at java.base/java.lang.Thread.run(Thread.java:834)
java.lang.NoClassDefFoundError: co/elastic/apm/agent/concurrent/JavaConcurrent
   at [email protected]//org.jboss.msc.service.ServiceContainerImpl$ContainerExecutor.execute(ServiceContainerImpl.java:946)
   at [email protected]//org.jboss.msc.service.ServiceControllerImpl.doExecute(ServiceControllerImpl.java:795)
   at [email protected]//org.jboss.msc.service.ServiceControllerImpl$ControllerTask.run(ServiceControllerImpl.java:1569)
   at [email protected]//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1982)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
   at [email protected]//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
   at java.base/java.lang.Thread.run(Thread.java:834)

@felixbarny
Copy link
Member Author

Runtime attachment was not supported on JBoss before this change

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.

5 participants