Add an agent plugin to support Jackson

[VictorZeng]

Add an agent plugin to support Jackson

• 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 [Feature] Add Jackson plugin skywalking#7812.
• Update the CHANGES log.

[kezhenxu94]

Apart from the comments inline, I'd also like to discuss whether we should also include part of the JSON string / bytes in the trace, for example, collect the first 64 bytes as a tag content and the 64 is configurable.

Currently we only have the time cost in (de)serializing and the length of the content, but it makes little sense that we don't know what the content is when the time cost is too long, including part of the content might give the users some hints what content makes the JSON lib slower.

[wu-sheng]

Apart from the comments inline, I'd also like to discuss whether we should also include part of the JSON string / bytes in the trace, for example, collect the first 64 bytes as a tag content and the 64 is configurable.

Currently we only have the time cost in (de)serializing and the length of the content, but it makes little sense that we don't know what the content is when the time cost is too long, including part of the content might give the users some hints what content makes the JSON lib slower.

Agree, collecting content is a risk. I prefer we don't do this.

[VictorZeng]

Apart from the comments inline, I'd also like to discuss whether we should also include part of the JSON string / bytes in the trace, for example, collect the first 64 bytes as a tag content and the 64 is configurable.
Currently we only have the time cost in (de)serializing and the length of the content, but it makes little sense that we don't know what the content is when the time cost is too long, including part of the content might give the users some hints what content makes the JSON lib slower.

Agree, collecting content is a risk. I prefer we don't do this.

I think that providing JSON length can satisfy most user scenarios. It is not recommended to collect JSON content.

[VictorZeng]

@kezhenxu94 use the guava ImmutableMap is a good idea, but failed the checkstyle.

[kezhenxu94]

@kezhenxu94 use the guava ImmutableMap is a good idea, but failed the checkstyle.

ok. We need some adjustments of the check style settings

[VictorZeng]

@kezhenxu94 use the ImmutableMap , must package the guava dependency with plugin jar together. otherwise:
Caused by: java.lang.ClassNotFoundException: Can't find com.google.common.collect.ImmutableMap

Is it necessary to do this?

[kezhenxu94]

@kezhenxu94 use the ImmutableMap , must package the guava dependency with plugin jar together. otherwise:

Caused by: java.lang.ClassNotFoundException: Can't find com.google.common.collect.ImmutableMap

Is it necessary to do this?

I think guava is bundled with agent core already

[VictorZeng]

@kezhenxu94 use the ImmutableMap , must package the guava dependency with plugin jar together. otherwise:
Caused by: java.lang.ClassNotFoundException: Can't find com.google.common.collect.ImmutableMap
Is it necessary to do this?

I think guava is bundled with agent core already

It doesn't look like this now. the apm-agent-core pom.xml guava in <dependencyManagement>

    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>com.google.guava</groupId>
                <artifactId>guava</artifactId>
                <version>${guava.version}</version>
            </dependency>
            ......
    </dependencyManagement>

and the apm-sdk-plugin and optional-plugins module pom,xml dependency apm-agent-core scope is provided

        <dependency>
            <groupId>org.apache.skywalking</groupId>
            <artifactId>apm-agent-core</artifactId>
            <version>${project.version}</version>
            <scope>provided</scope>
        </dependency>

[wu-sheng]

Using this 3rd party APIs, AFAIK, you need to shade. This dependency is there, but indirectly(gRPC depending on it) and shaded in the runtime.
Do we really need to make things this complex? Initializing ENHANCE_METHODS in constructor and use it should be good enough, right?

[VictorZeng]

Using this 3rd party APIs, AFAIK, you need to shade. This dependency is there, but indirectly(gRPC depending on it) and shaded in the runtime.
Do we really need to make things this complex? Initializing ENHANCE_METHODS in constructor and use it should be good enough, right?

I noticed this, the source code shaded in org.apache.skywalking.apm.dependencies.*
I think init the ENHANCE_METHODS in constructor is a good idea.

[wu-sheng]

Plugin default class loader is singleton and wouldn't be GC. Any new class file loaded will be a load for JVM memory permanently. Let's try to keep plugin codes as simple as possible.

[VictorZeng]

Plugin default class loader is singleton and wouldn't be GC. Any new class file loaded will be a load for JVM memory permanently. Let's try to keep plugin codes as simple as possible.

Use guava ImmutableMap is a better way, I think I can try to shade it.

[kezhenxu94]

@kezhenxu94 use the ImmutableMap , must package the guava dependency with plugin jar together. otherwise:
Caused by: java.lang.ClassNotFoundException: Can't find com.google.common.collect.ImmutableMap
Is it necessary to do this?

I think guava is bundled with agent core already

OK I remember it wrong, what is bundled is GSON dependency. But the idea is not to create too many anonymous inner classes, initializing the map in constructor is also good to me

[VictorZeng]

@wu-sheng @kezhenxu94 How about init ENHANCE_METHODS in static method block?

[kezhenxu94]

@wu-sheng @kezhenxu94 How about init ENHANCE_METHODS in static method block?

Okay to me

[VictorZeng]

@wu-sheng @kezhenxu94 How about init ENHANCE_METHODS in static method block?

Okay to me

Thank u.

[VictorZeng]

@kezhenxu94 elasticjob-3.x-scenario health check failed, it runs successfully on my local.

+ status=0
+ [[ 0 == 0 ]]
+ [[ -z '' ]]
+ rm -rf /home/skywalking-java/test/plugin/workspace/elasticjob-3.x-scenario/3.0.0
+ num_of_testcases=1
+ echo -e '\033[33melasticjob-3.x-scenario has already sumbitted\033[0m'
elasticjob-3.x-scenario has already sumbitted
+ exitAndClean 0
++ date +%s
+ elapsed=404
+ [[ 0 -eq 1 ]]
+ printf 'Scenarios: elasticjob-3.x-scenario, Testcases: 1, Elapsed: %02d:%02d:%02d \n' 0 6 44
Scenarios: elasticjob-3.x-scenario, Testcases: 1, Elapsed: 00:06:44 
+ exit 0

[kezhenxu94]

Thank you @VictorZeng . One nit: it seems that you didn't import our code style settings, the new files still don't respect to the code styles, please consider import our code style settings if you are going to contribute later

[VictorZeng]

Thank you @VictorZeng . One nit: it seems that you didn't import our code style settings, the new files still don't respect to the code styles, please consider import our code style settings if you are going to contribute later

Thank you for review, I will import the checkstyle.