Skip to content

Conversation

@wallezhang
Copy link
Contributor

Fix <#6730>

  • Add a unit test to verify that the fix works.
  • Explain briefly why the bug exists and how to fix it.

@wu-sheng
Copy link
Member

Should be reviewed by @codelipenghui

And you should update https://github.com/apache/skywalking/tree/master/test/plugin/scenarios/pulsar-scenario to verify your case. How to write plugin test doc is in the plugin dev doc already.

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #6774 (e9f48b3) into master (1aa5f26) will increase coverage by 0.48%.
The diff coverage is 70.58%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6774      +/-   ##
============================================
+ Coverage     53.75%   54.23%   +0.48%     
- Complexity     4156     4254      +98     
============================================
  Files          1832     1836       +4     
  Lines         39201    39379     +178     
  Branches       4336     4322      -14     
============================================
+ Hits          21071    21359     +288     
+ Misses        17110    17007     -103     
+ Partials       1020     1013       -7     
Impacted Files Coverage Δ Complexity Δ
.../plugin/pulsar/ConsumerConstructorInterceptor.java 90.00% <0.00%> (-10.00%) 0.00 <0.00> (ø)
...m/plugin/pulsar/MessageConstructorInterceptor.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...g/apm/plugin/pulsar/PulsarConsumerInterceptor.java 81.25% <72.72%> (-4.47%) 0.00 <0.00> (ø)
...ugin/pulsar/PulsarConsumerListenerInterceptor.java 80.00% <80.00%> (ø) 0.00 <0.00> (?)
...org/apache/skywalking/apm/toolkit/meter/Gauge.java 25.00% <0.00%> (-75.00%) 0.00% <0.00%> (ø%)
...ent/core/plugin/bytebuddy/ReturnTypeNameMatch.java 50.00% <0.00%> (-50.00%) 0.00% <0.00%> (ø%)
...king/apm/agent/core/context/util/KeyValuePair.java 36.36% <0.00%> (-43.64%) 0.00% <0.00%> (ø%)
...core/plugin/bytebuddy/AnnotationTypeNameMatch.java 57.14% <0.00%> (-42.86%) 0.00% <0.00%> (ø%)
...ng/apm/agent/core/context/trace/LogDataEntity.java 72.22% <0.00%> (-27.78%) 0.00% <0.00%> (ø%)
...pache/skywalking/apm/agent/core/meter/MeterId.java 36.84% <0.00%> (-26.80%) 0.00% <0.00%> (ø%)
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aa5f26...e9f48b3. Read the comment docs.

@wallezhang
Copy link
Contributor Author

Should be reviewed by @codelipenghui

And you should update https://github.com/apache/skywalking/tree/master/test/plugin/scenarios/pulsar-scenario to verify your case. How to write plugin test doc is in the plugin dev doc already.

OK, I will update pulsar-scenario according to the doc

@wallezhang wallezhang force-pushed the apm-pulsar-plugin-add-listener-instrument branch from 7713e55 to 0c6c2d3 Compare April 17, 2021 02:14
@wu-sheng
Copy link
Member

If you add a new consumer, you need to update the expected data file.

@wu-sheng
Copy link
Member

And Changes.md should be updated to describe what you changed.

@wu-sheng
Copy link
Member

pulsar plugin test fails, please verify the tests locally first.

@wu-sheng wu-sheng added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Apr 17, 2021
@wallezhang
Copy link
Contributor Author

@codelipenghui I found that byte buddy can not instrument lambda, however most cases when we set a MessageListener, we use lambda expression, so I wonder if there is a better way to instrument the MessageListener.

@wu-sheng
Copy link
Member

I found that byte buddy can not instrument lambda

This is not related to byte-buddy. It can do, but SkyWalking doesn't want you to do that. The recommended way to do is enhancing the method using the Lambda, and create another new lambda as a wrapper to do the instrument.

@wallezhang
Copy link
Contributor Author

I found that byte buddy can not instrument lambda

This is not related to byte-buddy. It can do, but SkyWalking doesn't want you to do that. The recommended way to do is enhancing the method using the Lambda, and create another new lambda as a wrapper to do the instrument.

OK, Just like what the class RunnableWrapper does?

@wu-sheng
Copy link
Member

I found that byte buddy can not instrument lambda

This is not related to byte-buddy. It can do, but SkyWalking doesn't want you to do that. The recommended way to do is enhancing the method using the Lambda, and create another new lambda as a wrapper to do the instrument.

OK, Just like what the class RunnableWrapper does?

Yes, but use a new lambda. I remember we have this in some plugins, but can't tell which. Sorry, too many plugins.

@wallezhang wallezhang force-pushed the apm-pulsar-plugin-add-listener-instrument branch from e16596c to 1f50357 Compare April 25, 2021 03:12
@wallezhang
Copy link
Contributor Author

@codelipenghui Sorry for late update. I instrument getMessageListener method in org.apache.pulsar.client.impl.conf.ConsumerConfigurationData class, create a new lambda expression wrap the original message listener if it exists. And instrument Message interface so that I can pass the trace context snapshot to message listener thread. Plz take a review.

@wallezhang
Copy link
Contributor Author

@wu-sheng During the local test, I found that if I want to run unit test locally, I had to introduce some extra libraries in the root module pom file, e.g. powermock-module-junit4-common, powermock-api-support, mockito-core. Did I miss some configuration?

@wu-sheng
Copy link
Member

@wu-sheng During the local test, I found that if I want to run unit test locally, I had to introduce some extra libraries in the root module pom file, e.g. powermock-module-junit4-common, powermock-api-support, mockito-core. Did I miss some configuration?

About the libraries you mentioned, some are not included. But we have included some libraries about mockito-core is there. I am not sure why you can't use it.

And you seems to update many submodules(core proto, query-protocol, test proto and UI), which should not be. Please revert these unexpected changes.

@wallezhang
Copy link
Contributor Author

@wu-sheng During the local test, I found that if I want to run unit test locally, I had to introduce some extra libraries in the root module pom file, e.g. powermock-module-junit4-common, powermock-api-support, mockito-core. Did I miss some configuration?

About the libraries you mentioned, some are not included. But we have included some libraries about mockito-core is there. I am not sure why you can't use it.

And you seems to update many submodules(core proto, query-protocol, test proto and UI), which should not be. Please revert these unexpected changes.

Sorry, I have reverted these changes already

@wu-sheng
Copy link
Member

@wu-sheng During the local test, I found that if I want to run unit test locally, I had to introduce some extra libraries in the root module pom file, e.g. powermock-module-junit4-common, powermock-api-support, mockito-core. Did I miss some configuration?

About the libraries you mentioned, some are not included. But we have included some libraries about mockito-core is there. I am not sure why you can't use it.
And you seems to update many submodules(core proto, query-protocol, test proto and UI), which should not be. Please revert these unexpected changes.

Sorry, I have reverted these changes already

You reverted more than expected. Please recheck the file changes. Many things are being removed.

@wallezhang wallezhang force-pushed the apm-pulsar-plugin-add-listener-instrument branch from 0191d34 to 78b5cc1 Compare April 25, 2021 06:58
@wallezhang
Copy link
Contributor Author

@wu-sheng During the local test, I found that if I want to run unit test locally, I had to introduce some extra libraries in the root module pom file, e.g. powermock-module-junit4-common, powermock-api-support, mockito-core. Did I miss some configuration?

About the libraries you mentioned, some are not included. But we have included some libraries about mockito-core is there. I am not sure why you can't use it.
And you seems to update many submodules(core proto, query-protocol, test proto and UI), which should not be. Please revert these unexpected changes.

Sorry, I have reverted these changes already

You reverted more than expected. Please recheck the file changes. Many things are being removed.

I make some new commit, and recheck the change file list. All files are only related to this PR. 😄

@wu-sheng
Copy link
Member

I gave the green light to CI. @codelipenghui please review the codes.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Forward @codelipenghui 's approval, as he wrote this. And once tests passed, I am good with this PR.

@wu-sheng wu-sheng added this to the 8.6.0 milestone Apr 26, 2021
@wallezhang
Copy link
Contributor Author

Great! Thank all for helping to review the code and put forward suggestions. 😄

@wu-sheng
Copy link
Member

@wallezhang Welcome to join the code contributor list. You will find your GitHub ID on https://skywalking.apache.org/team/

@wu-sheng wu-sheng merged commit 9de3724 into apache:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pulsar plugin breaks trace on the consumer size

3 participants