Skip to content

add plugin for dubbo3#73

Merged
wu-sheng merged 1 commit intoapache:mainfrom
tedli:dubbo3
Dec 10, 2021
Merged

add plugin for dubbo3#73
wu-sheng merged 1 commit intoapache:mainfrom
tedli:dubbo3

Conversation

@tedli
Copy link
Copy Markdown
Contributor

@tedli tedli commented Nov 23, 2021

Add an agent plugin to support dubbo3

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

@wu-sheng
Copy link
Copy Markdown
Member

A new plugin test(not UT) is required like this, https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/dubbo-2.7.x-scenario.

We have plugin development doc to guide you how to write a plugin test.

@tedli tedli marked this pull request as draft November 23, 2021 07:19
@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 24, 2021

If not, read this, https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/java-plugin-development-guide/#implement-plugin, search this 4. Set up witnessClasses and/or witnessMethods if the instrumentation has to be activated in specific versions.

witness classes and methods added, and extract common code of both dubbo 2.7 and 3 into individual maven module.

A new plugin test(not UT) is required like this, https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/dubbo-2.7.x-scenario.

This is on the road...

Comment thread apm-sniffer/apm-sdk-plugin/dubbo-commons/pom.xml Outdated
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Nov 24, 2021

witness classes and methods added, and extract common code of both dubbo 2.7 and 3 into individual maven module.

The dubbo plugin is not very complex like JDBC, do we really need the common package and set up this complex hierarchy structure? Dubbo v2 will dead(EOL) one day, keeping them separate will make us easier to remove Dubbo 2 plugin and keep 3 plugin w/o change.

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 24, 2021

witness classes and methods added, and extract common code of both dubbo 2.7 and 3 into individual maven module.

The dubbo plugin is not very complex like JDBC, do we really need the common package and set up this complex hierarchy structure? Dubbo v2 will dead(EOL) one day, keeping them separate will make us easier to remove Dubbo 2 plugin and keep 3 plugin w/o change.

Ok, rollback.

TODO:

  • add witness to tell is dubbo v2.7 or v3
  • a new plugin test(not UT)

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 25, 2021

[2021-11-25 07:08:58:225] [ERROR] - org.apache.skywalking.plugin.test.agent.tool.Main.verify(Main.java:53) -
assert failed.
SegmentSizeNotEqualsException: dubbo-2.7.x-scenario
expected: ge 3
actual: 2

Current state of main branch (without this pr), I can't get plugin test of dubbo 2.7.x pass.

# by using this command
./test/plugin/run.sh -f dubbo-2.7.x-scenario

Turns out that I didn't make build first. Test (2.7.x) passed.

@wu-sheng
Copy link
Copy Markdown
Member

Have you passed maven install and both plugin cases locally?

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 26, 2021

Have you passed maven install and both plugin cases locally?

Yes, it passed (without this pr). With this pr I can pass other scenarios except dubbo2.7 and 3. So it turns out I broke something.

@wu-sheng
Copy link
Copy Markdown
Member

I think it is better you reset anything back to main branch, then add your new plugin implementation with witness class.

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 29, 2021

I think it is better you reset anything back to main branch, then add your new plugin implementation with witness class.

Yes, I did exactly the same thing. Checked out the main branch, add witness class only to dubbo2.7 and it's conflict patch, do nothing with dubbo3.

Then the 2.7 scenario test broke. However, with these witness class, my 2.7 demo and 3 demo works as expect (the log file in logs folder shows that 2.7 demo dosen't meet 3's witness, and vice versa, and agent can produce spans which appear on skywalking ui as expect).

It needs some more time, I will keep on working on this.

@wu-sheng
Copy link
Copy Markdown
Member

Thanks for updating. If there is any help we could provide, please let us know.

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Nov 30, 2021

I got the point why the scenario test failed. It's because the chosen witness classes fit one minor (patch) version but not for other minor (patch) versions.
The dubbo core classes are stable among various major version like org.apache.dubbo.rpc.RpcContext, however it can't be used to tell it's dubbo 2.7.x or 3.
Meanwhile other classes like org.apache.dubbo.rpc.protocol.rest.NettyServer, exists in 2.7.x not in 3. But in some newer patch version of 2.7.x, it also not exists.
So it takes more time to find classes which could be used to determine it's 2.7.x or 3.x, and such classes should be intuitively stay stable among all the 'x' minor or patch versions.

@wu-sheng
Copy link
Copy Markdown
Member

We had a contributed contributing this tool to help you locate the witness, https://github.com/SkyAPM/uranus.

BTW, it doesn't have to be a witness class, a method signature is also works.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 6, 2021

@tedli Any luck with this plugin?

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Dec 6, 2021

@tedli Any luck with this plugin?

Hi, the witness methods works fine, by adding witness methods like this:

// dubbo 2.7.x
@Override
protected List<WitnessMethod> witnessMethods() {
    return Collections.singletonList(new WitnessMethod(
            "org.apache.dubbo.rpc.RpcContext",
            named("getServerContext").and(returns(named("org.apache.dubbo.rpc.RpcContext")))));
}

// dubbo 3.x
@Override
protected List<WitnessMethod> witnessMethods() {
    return Collections.singletonList(new WitnessMethod(
            "org.apache.dubbo.rpc.RpcContext",
            named("getServerContext").and(returns(named("org.apache.dubbo.rpc.RpcContextAttachment")))));
}

the agent mechanism accurately load the correct plugin. And the scenario test of 2.7.x passed as it is.

How ever the scenario test of 3 still failed, it because request url got from invocation seemed weird, the produced span didn't match the expect value.

It need some further investigation about how to construct a span for dubbo 3.

😭 It's the end of the year, I have to handle some of my own mess, which has higher priority. I'm very glad to continue working on this issue. I will make some time for this.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 6, 2021

Got it. Make your time, no hurry about this. I just want to check the status, and we will plan to release the next 8.9.0 for agent after we have Dubbo plugin. Because we know people at China love Dubbo project, and would require this plugin.

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Dec 7, 2021

Hi, @wu-sheng

FYI

https://github.com/apache/dubbo/blob/00ca43643d243f0866389abd294c6160e155c8a5/dubbo-monitor/dubbo-monitor-api/src/main/java/org/apache/dubbo/monitor/support/MonitorFilter.java#L54-L55

https://github.com/apache/dubbo/blob/5a5e3f3fd7d0482b6124bd5b52b10f4900856438/dubbo-monitor/dubbo-monitor-api/src/main/java/org/apache/dubbo/monitor/support/MonitorFilter.java#L52-L53

// dubbo 2.7.x
@Activate(group = {PROVIDER, CONSUMER})
public class MonitorFilter implements Filter, Filter.Listener {
}

// dubbo 3.x
@Activate(group = {PROVIDER})
public class MonitorFilter implements Filter, Filter.Listener {
}

After some debugging, I found out why the scenario test of dubbo3 failed, it's because the occasion that MonitorFilter takes place is different in dubbo 3.x from 2.7.x, only provider side produced spans. So the test failed.

It need to find other filters to adjust instrumentation class intercepting behavior, so that consumer side can also produce spans.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 7, 2021

Is any CONSUMER filter available? How monitoring works for CONSUMER side Dubbo3?

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Dec 7, 2021

Is any CONSUMER filter available? How monitoring works for CONSUMER side Dubbo3?

Yes, there is.

https://github.com/apache/dubbo/blob/dubbo-3.0.4/dubbo-monitor/dubbo-monitor-api/src/main/java/org/apache/dubbo/monitor/support/MonitorClusterFilter.java

I notice that some other plugins has different instrumentation classes for server and client like the grpc plugin.

I'm trying to add an instrumentation class for dubbo 3 consumer in the same way.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 7, 2021

Sure, you could disable the whole MonitorFilter-based instrumentation for Dubbo3, and pick a better one.

@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Dec 9, 2021

Hi @wu-sheng ,

I found that if using the same scenario test code like dubbo 2.7.x, the consumer proxy object will use an InJvmInvoker, or if explicitly set inJvm to false, a DubboInvoker will be used, neither of them loads filters.

Then I ran a EmbeddedZooKeeper, by using a registry,

https://github.com/apache/dubbo/blob/5a5e3f3fd7d0482b6124bd5b52b10f4900856438/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/filter/ProtocolFilterWrapper.java#L68-L75

@Override
public <T> Invoker<T> refer(Class<T> type, URL url) throws RpcException {
    if (UrlUtils.isRegistry(url)) {
        return protocol.refer(type, url);
    }
    FilterChainBuilder builder = getFilterChainBuilder(url);
    return builder.buildInvokerChain(protocol.refer(type, url), REFERENCE_FILTER_KEY, CommonConstants.CONSUMER);
}

the consumer proxy object loads filters. So skywalking interceptor on consumer side affects. The produced spans are as expect.

segmentItems:
- serviceName: dubbo-3.x-scenario
  segmentSize: 3
  segments:
  - segmentId: 42afe68bf5ad435787439867ed63a554.64.16390270335870000
    spans:
    - operationName: HEAD:/dubbo-3.x-scenario/case/healthCheck
      operationId: 0
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1639027033589
      endTime: 1639027033684
      componentId: 1
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/dubbo-3.x-scenario/case/healthCheck'}
      - {key: http.method, value: HEAD}
  - segmentId: 42afe68bf5ad435787439867ed63a554.86.16390270340630000
    spans:
    - operationName: org.apache.skywalking.apm.testcase.dubbo3.services.GreetService.doBusiness(String)
      operationId: 0
      parentSpanId: -1
      spanId: 0
      spanLayer: RPCFramework
      startTime: 1639027034063
      endTime: 1639027034068
      componentId: 3
      isError: false
      spanType: Entry
      peer: 172.17.0.2:40328
      skipAnalysis: false
      tags:
      - {key: url, value: 'dubbo://172.17.0.2:20080/org.apache.skywalking.apm.testcase.dubbo3.services.GreetService.doBusiness(String)'}
      - {key: arguments, value: helloWorld}
      refs:
      - {parentEndpoint: 'GET:/dubbo-3.x-scenario/case/dubbo', networkAddress: '172.17.0.2:0',
        refType: CrossProcess, parentSpanId: 1, parentTraceSegmentId: 42afe68bf5ad435787439867ed63a554.65.16390270336920000,
        parentServiceInstance: ae9af82e06fe419097b7483b8e2d6522@172.17.0.2, parentService: dubbo-3.x-scenario,
        traceId: 42afe68bf5ad435787439867ed63a554.65.16390270336920001}
  - segmentId: 42afe68bf5ad435787439867ed63a554.65.16390270336920000
    spans:
    - operationName: org.apache.skywalking.apm.testcase.dubbo3.services.GreetService.doBusiness(String)
      operationId: 0
      parentSpanId: 0
      spanId: 1
      spanLayer: RPCFramework
      startTime: 1639027033946
      endTime: 1639027034073
      componentId: 3
      isError: false
      spanType: Exit
      peer: 172.17.0.2:0
      skipAnalysis: false
      tags:
      - {key: url, value: 'dubbo://172.17.0.2:0/org.apache.skywalking.apm.testcase.dubbo3.services.GreetService.doBusiness(String)'}
      - {key: arguments, value: helloWorld}
    - operationName: GET:/dubbo-3.x-scenario/case/dubbo
      operationId: 0
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1639027033692
      endTime: 1639027034075
      componentId: 1
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/dubbo-3.x-scenario/case/dubbo'}
      - {key: http.method, value: GET}

However, because a registry is used, the address no longer be the localhost and depends. So the expectedData.yaml can't keep using localhost.

And I didn't find something like startWith, contains, endWith in the skywalking-agent-test-tool, can I simply use not null?

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 9, 2021

And I didn't find something like startWith, contains, endWith in the skywalking-agent-test-tool, can I simply use not null?

Yes, that is fine.

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 9, 2021

You need to add your case into GHA control file, then I could activate CI for you.

@tedli tedli force-pushed the dubbo3 branch 2 times, most recently from 4f07c82 to a6073e9 Compare December 9, 2021 08:05
@tedli tedli marked this pull request as ready for review December 9, 2021 08:07
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 9, 2021

Supported-list.md, Plugin-list.md and changes.md should be updated, otherwise, the CI would fail.

@tedli tedli force-pushed the dubbo3 branch 2 times, most recently from 07efff8 to 1c35946 Compare December 9, 2021 08:47
@tedli
Copy link
Copy Markdown
Contributor Author

tedli commented Dec 9, 2021

Supported-list.md, Plugin-list.md and changes.md should be updated, otherwise, the CI would fail.

I saw that the checking for Plugin-list.md reads skywalking-plugin.def, I don't know how this file work. If I add new entry for dubbo3, should I adjust content of skywalking-plugin.def to something like this?

dubbo3.x=org.apache.skywalking.apm.plugin.asf.dubbo3.DubboProviderInstrumentation
dubbo3.x=org.apache.skywalking.apm.plugin.asf.dubbo3.DubboConsumerInstrumentation

By the way, I saw retransform-class-scenario failed in previous CI, did I broke it?

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 9, 2021

The name of skywalking-plugin.def provides the bootstrap configurable capabilities to disable this plugin directly though config file or system env, rather than removing jar files. So it is better to rename preview to dubbo2.x and add dubbo3.x

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 9, 2021

retransform case should only fail unrelatedly. I guess. I can rerun if it happens again. Don't need to worry for now.

Comment thread test/plugin/scenarios/dubbo-3.x-scenario/pom.xml Outdated
@wu-sheng wu-sheng added this to the 8.9.0 milestone Dec 10, 2021
@wu-sheng wu-sheng merged commit ee5bb80 into apache:main Dec 10, 2021
@Cuihongsen
Copy link
Copy Markdown

When will a new version support this new feature be released

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Jan 6, 2022

In this month, maybe. We are waiting for a grpc next release.

@wu-sheng wu-sheng mentioned this pull request Jan 24, 2022
2 tasks
@honganan honganan mentioned this pull request Feb 24, 2022
2 tasks
GuoHaoZai pushed a commit to GuoHaoZai/skywalking-java that referenced this pull request Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants