Add WebSphere Liberty 23.x plugin.(#10887) #560

weixiang1862 · 2023-06-21T00:45:27Z

Add an agent plugin to support WebSphere Liberty

Add a test case for the new plugin, refer to the doc
Add a component id in the component-libraries.yml
Add a logo in the UI repo

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10887
Update the CHANGES log.

…f(#10887)

wu-sheng · 2023-06-21T03:29:11Z

.../java/org/apache/skywalking/apm/plugin/websphereliberty/v23/CompleteRunnableInterceptor.java

+ @Override
+ public void onConstruct(final EnhancedInstance objInst, final Object[] allArguments) throws Throwable {
+ // add special flag for AsyncContext distinguish
+ objInst.setSkyWalkingDynamicField(AsyncType.COMPLETE);


This is a class level attribute. Is this class always staying in asyc completed? No race condition?

yes, the CompleteRunnable is created in AsyncContextImpl.complete() method with AsyncType.COMPLETE attribute and then it will be submit to an executor by AsyncContextImpl.startWithExecutorThread(). There are no race condition in the whole process, set and get this attribute is always in one thread.

IBM open source liberty code but not release a maven dependency(I can not find it on maven). CompleteRunnable and DispatchRunnable is not reachable in agent plugin code. That is why I use this strange way rather than xxx instanceof CompleteRunnable.

You could write classes(fake claases) in weblogic namespace and exclude them in the package stage, then, your codes could work without instanceof and have higher perf.

That sounds interesting, I will have a try.

Notice the license of those fake files. If you only fake some simple methods, it is fine, but ve clear that they are fake classes and not distributed in their class comments.

I put fake class file under org.apache.skywalking.apm.plugin.webspheresource.com.ibm.ws.webcontainer.async.

maven-jar-plugin exclude it from final jar and maven-shade-plugin relocation the packagename to com.ibm.ws.webcontainer.async.

Please help me review the code.

Please try this, list all your com.ibm.ws.webcontainer.async.*(all explicit class names) in here,

skywalking-java/pom.xml

Line 411 in b72e984

<excludes>

I hope this could work and avoid shade.

OK, I will try this.

wu-sheng · 2023-06-21T03:31:25Z

...g/apache/skywalking/apm/plugin/websphereliberty/v23/JavaxFilterManagerInvokeInterceptor.java

+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+ if (ContextManager.isActive()) {


Is there a case not active? I noticed you added this check in every place. Notice that isActive has resource cost too. We only should check when it is expected to be NOT active in some specific known cases.

You are rignt, This check is no necessary. I will remove it.

…ty plugin (#10887)

# Conflicts: # CHANGES.md

wu-sheng · 2023-06-21T08:35:11Z

I am a little confused, why shade?

wu-sheng · 2023-06-21T08:39:02Z

com.ibm should be in codes, and you should exclude those(com.ibm.*) when package.
Could you explain your way?

weixiang1862 · 2023-06-21T08:44:51Z

I am a little confused, why shade?

Checkstyle requires sourcecode package name must start with org.apache.skywalking.apm.plugin, so i put websphere fake class under org.apache.skywalking.apm.plugin.webspheresource.com.ibm.ws.webcontainer.async.

If not shade, websphere runtime will throw java.lang.NoClassDefFoundError for org.apache.skywalking.apm.plugin.webspheresource.com.ibm.ws.webcontainer.async.CompleteRunnable.class missing

weixiang1862 · 2023-06-21T08:47:54Z

com.ibm should be in codes, and you should exclude those(com.ibm.*) when package. Could you explain your way?

I use maven-jar-plugin, include codes except fackclass

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-jar-plugin</artifactId>
    <configuration>
        <includes>
            <include>org/apache/skywalking/apm/plugin/websphereliberty/**/*</include>
            <include>META-INF/**/*</include>
            <include>skywalking-plugin.def</include>
        </includes>
    </configuration>
</plugin>

wu-sheng · 2023-06-21T09:28:14Z

...main/java/org/apache/skywalking/apm/plugin/websphereliberty/v23/AsyncContextInterceptor.java

+ MethodInterceptResult result) throws Throwable {
+ Runnable runnable = (Runnable) allArguments[0];
+
+ if (runnable instanceof RunnableWrapper) {


About this instanceof, is this executed more than once?

Yes. In websphere liberty, start runnable created by AsyncContext.start() method finally will be submit to an executor by AsyncContext.startWithExecutorThread method. So enhance startWithExecutorThread is enough.

But in websphere traditional edition, start method has a check, if WSAsyncContextImpl.wcConfig.isUseAsyncRunnableWorkManager()==true, start runnable cant not enter startWithExecutorThread method. So we must enhance start method too.

Both enhance start & startWithExecutorThread will cause AsyncContextInterceptor run twice. So I have a instanceof RunnableWrapper check.

xu1009 · 2023-06-21T10:01:21Z

...e/skywalking/apm/plugin/websphereliberty/v23/define/JakartaFilterManagerInstrumentation.java

+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return named("invokeFilters")
+ .and(takesArgument(0, named("jakarta.servlet.ServletRequest")))
+ .and(takesArgument(1, named("jakarta.servlet.ServletResponse")));


com.ibm.ws.webcontainer.filter.WebAppFilterManager.invokeFilters have different arguments in same version websphere?

it seems javax and jakarta are in diiferent version websphere, i think it is better to develop plugins for different version websphere, you can overwrite witnessMethods to identify it

Yes, it is different version of websphere. WebSphere liberty use jakarta and webSphere traditional use javax.

Currnetly WebSphere Liberty 23.x plugin compatible for webSphere liberty and webSphere traditional.

You means I should create another plugin 'websphere-traditional-xx-plugin'？

I feel like javax version of codes is not tested through the scenario. @weixiang1862 Could you confirm the status?

Yes, it is different version of websphere. WebSphere liberty use jakarta and webSphere traditional use javax.

Currnetly WebSphere Liberty 23.x plugin compatible for webSphere liberty and webSphere traditional.

You means I should create another plugin 'websphere-traditional-xx-plugin'？

@weixiang1862 How do you make sure these two kinds of APIs are compiled in one plugin? Just because they have different package names? But how do you test?

I think @xu1009 's point is witness class is always recommended to do a version differentiation. Your way works too but a little tricky and implicit.
witness + 2 plugins or 2 package names should be better. Also, you should mention this on the plugin docs.

I feel like javax version of codes is not tested through the scenario. @weixiang1862 Could you confirm the status?

Yes，I test this case on websphere traditional 8.5.5 & 9.0.5 in my local. It is a little bit troublesome to create a websphere traditional scenario. Currently there are jvm & tomcat container for scenario test, May be I shoud create a websphere container type？

If it is EPLv2 as well, we could do that.

Notice, that(new container) should be another new PR.

@weixiang1862 For this PR, please separate traditional and jakata in two plugins with a parent module websphere-liberty

@wu-sheng @xu1009 OK, I will do the separate jobs these days.

By the way, Happy Dragon Boat Festival!

You too. Have a good vacation

wu-sheng · 2023-06-21T12:14:28Z

apm-sniffer/apm-sdk-plugin/websphere-liberty-23.x-plugin/pom.xml

+ <artifactId>maven-jar-plugin</artifactId>
+ <configuration>
+ <includes>
+ <include>org/apache/skywalking/apm/plugin/websphereliberty/**/*</include>


Suggested change

<include>org/apache/skywalking/apm/plugin/websphereliberty/**/*</include>

<include>org/apache/skywalking/apm/plugin/websphereliberty/v23/**</include>

And move the fake classes into org/apache/skywalking/apm/plugin/websphereliberty/mock.

…(#10887)

weixiang1862 · 2023-06-25T05:07:55Z

@wu-sheng @xu1009 I made the following changes in this commit:

move mock class to com.ibm.** package and add them to check-style exclude list
use com.ibm.ws.webcontainer.WebContainer.handleRequest(IRequest, IResponse) as interceptor point, it is high level class and hided the detail of real implement(javax or jakarta). So the plugin still keep one.

wu-sheng · 2023-06-25T05:54:56Z

move mock class to com.ibm.** package and add them to check-style exclude list

I can't find it in the change log. There are the changes?

weixiang1862 · 2023-06-25T06:01:39Z

move mock class to com.ibm.** package and add them to check-style exclude list

I can't find it in the change log. There are the changes?

Yes, mock classes are currently in com.ibm.** packages and I add these mock classes to maven-checkstyle-plugin exclude list

pom.xml

wu-sheng

LGTM. Pending on @xu1009 recheck.

weixiang1862 added 2 commits June 20, 2023 22:37

Add WebSphere Liberty 23.x plugin.(#10887)

8d31ddb

Add WebSphere Liberty 23.x test scenario(#10887)

d129adc

wu-sheng requested a review from xu1009 June 21, 2023 01:26

wu-sheng added plugin feature labels Jun 21, 2023

weixiang1862 added 2 commits June 21, 2023 09:33

Add missing license for websphere-liberty-23.x-plugin/pom.xml (#10887)

e970660

fix plugin name in websphere-liberty-23.x-plugin skywalking-plugin.de…

c1fe96a

…f(#10887)

wu-sheng added this to the 8.17.0 milestone Jun 21, 2023

wu-sheng reviewed Jun 21, 2023

View reviewed changes

weixiang1862 added 3 commits June 21, 2023 13:38

remove unnecessary ContextManager activation check in websphere-liber…

0ae0577

…ty plugin (#10887)

remove CompleteRunnable & DispatchRunnable enhance(#10887)

c6110c8

Merge remote-tracking branch 'origin/main'

7611639

# Conflicts: # CHANGES.md

wu-sheng reviewed Jun 21, 2023

View reviewed changes

xu1009 reviewed Jun 21, 2023

View reviewed changes

wu-sheng reviewed Jun 21, 2023

View reviewed changes

weixiang1862 added 2 commits June 25, 2023 11:45

Merge remote-tracking branch 'origin/main'

be31ef9

use WebContainer.handleRequest as interceptor point for http tracing.…

7ad4a16

…(#10887)

wu-sheng reviewed Jun 25, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

Update pom.xml

56761c9

wu-sheng requested a review from xu1009 June 25, 2023 06:21

wu-sheng approved these changes Jun 25, 2023

View reviewed changes

wu-sheng merged commit e464f18 into apache:main Jun 25, 2023
158 of 174 checks passed

yangyulely pushed a commit to yangyulely/skywalking-java that referenced this pull request May 25, 2024

Add WebSphere Liberty 23.x plugin.(#10887) (apache#560)

6c53983

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebSphere Liberty 23.x plugin.(#10887) #560

Add WebSphere Liberty 23.x plugin.(#10887) #560

weixiang1862 commented Jun 21, 2023 •

edited

Loading

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng commented Jun 21, 2023

wu-sheng commented Jun 21, 2023

weixiang1862 commented Jun 21, 2023

weixiang1862 commented Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

xu1009 Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

wu-sheng Jun 21, 2023

wu-sheng Jun 21, 2023

weixiang1862 Jun 21, 2023

wu-sheng Jun 21, 2023

wu-sheng Jun 21, 2023 •

edited

Loading

weixiang1862 commented Jun 25, 2023

wu-sheng commented Jun 25, 2023

weixiang1862 commented Jun 25, 2023

wu-sheng left a comment

	<include>org/apache/skywalking/apm/plugin/websphereliberty/*/</include>
	<include>org/apache/skywalking/apm/plugin/websphereliberty/v23/**</include>

Add WebSphere Liberty 23.x plugin.(#10887) #560

Add WebSphere Liberty 23.x plugin.(#10887) #560

Conversation

weixiang1862 commented Jun 21, 2023 • edited Loading

Add an agent plugin to support WebSphere Liberty

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented Jun 21, 2023

wu-sheng commented Jun 21, 2023

weixiang1862 commented Jun 21, 2023

weixiang1862 commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

weixiang1862 commented Jun 25, 2023

wu-sheng commented Jun 25, 2023

weixiang1862 commented Jun 25, 2023

wu-sheng left a comment

Choose a reason for hiding this comment

weixiang1862 commented Jun 21, 2023 •

edited

Loading

wu-sheng Jun 21, 2023 •

edited

Loading