Fix deserialization of kafka producer json config in the kafka-reporter-plugin. #542

gzlicanyi · 2023-05-31T08:29:36Z

Fix

Add a unit test to verify that the fix works.
Explain briefly why the bug exists and how to fix it.
When the value of a JSON config in Kafka is a non-string type, such as {"batch.size":32768}, an error will be thrown when setting properties with the generated map.

ERROR 2023-05-31 14:56:57.847 SkywalkingAgent-13-org.apache.skywalking.apm.dependencies.kafkaProducerInitThread-0 KafkaProducerManager : unexpected exception. 
java.lang.ClassCastException: java.lang.Double cannot be cast to java.lang.String
        at java.util.Map.forEach(Map.java:630)
        at org.apache.skywalking.apm.agent.core.kafka.KafkaProducerManager.run(KafkaProducerManager.java:111)

Because this code does not deserialize the value of JSON into a String type. However, the value required by Properties is of String type.

Map<String, String> config = (Map<String, String>) gson.fromJson(Kafka.PRODUCER_CONFIG_JSON, Map.class);

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

…lugin.

…er-plugin.

wu-sheng · 2023-05-31T09:16:23Z

I think this is duplicated with #540?

gzlicanyi · 2023-05-31T09:27:17Z

#540 hasn't fixed this bug.

wu-sheng · 2023-05-31T09:32:35Z

Didn't you have fixed? #538 I am confused.

gzlicanyi · 2023-05-31T09:36:22Z

Didn't you have fixed? #538 I am confused.

It's not the same issue. #538 fixed the Gson dependency issue, while #542 fixed the Gson deserialization problem, but both addressed the Kafka JSON config issue.

gzlicanyi · 2023-05-31T09:39:07Z

Didn't you have fixed? #538 I am confused.

When the value of a JSON config in Kafka is a non-string type, such as {"batch.size":32768}, an error will be thrown when setting properties with the generated map.

But such as {"batch.size":"32768"}, it is ok.

wu-sheng · 2023-05-31T09:42:34Z

Could you provide more details? I am not familiar with this kind of Kafka configuration.

gzlicanyi · 2023-05-31T09:47:53Z

Could you provide more details? I am not familiar with this kind of Kafka configuration.

Kafka's batch.size configuration refers to the size of messages sent by the producer in one batch, i.e., the batch size. By default, the batch.size is 16KB. If the size of a message is smaller than batch.size, the producer will wait for a certain amount of time to try to aggregate more messages before sending them together, in order to improve producer efficiency.

Typically, the batch size can be adjusted based on the size of messages sent by the producer to achieve optimal performance. If the messages are small, the batch size can be decreased, and if the messages are large, the batch size can be increased. However, it should be noted that if the batch size is set too large, it can increase the time it takes for messages to be transmitted over the network and increase the risk of message loss.

batch.size is usually configured as a numeric value.

wu-sheng · 2023-05-31T09:50:23Z

...lugin/src/test/java/org/apache/skywalking/apm/agent/core/kafka/KafkaProducerManagerTest.java

+    @Test
+    public void testSetPropertiesFromJsonConfig() {
+        KafkaProducerManager kafkaProducerManager = new KafkaProducerManager();
+        Properties properties = new Properties();
+
+        KafkaReporterPluginConfig.Plugin.Kafka.PRODUCER_CONFIG_JSON = "{\"batch.size\":32768}";
+        kafkaProducerManager.setPropertiesFromJsonConfig(properties);
+
+        assertEquals(properties.get("batch.size"), "32768");
+    }
+


What does this UT prove? The codes are not shaded ever in the UT scope, right?

Show an example of how a JSON configuration can be correctly set to properties.

What does this UT prove? The codes are not shaded ever in the UT scope, right?

right.

So, the point of this PR is TypeToken, which cast the type accordingly.

But still import com.google.common.reflect.TypeToken; seems not from gson package. Why?

So, the point of this PR is TypeToken, which cast the type accordingly.

But still import com.google.common.reflect.TypeToken; seems not from gson package. Why?

I have used the wrong TypeToken out of two.

wu-sheng · 2023-05-31T09:52:49Z

apm-sniffer/optional-reporter-plugins/kafka-reporter-plugin/pom.xml

+                                    <pattern>com.google.gson</pattern>
+                                    <shadedPattern>${shade.package}.com.google.common</shadedPattern>
+                                </relocation>


com.google.gson -> com.google.common?

wu-sheng · 2023-05-31T10:09:17Z

Please fix the conflicts as another PR gets merged.

…er-plugin. (apache#542)

gzlicanyi added 6 commits May 26, 2023 16:53

Fix the gson dependency in the kafka-reporter-plugin. (#10848)

daf58f3

Fix the gson dependency by maven-shade-plugin in the kafka-reporter-p…

9a1ad5d

…lugin.

Merge branch 'apache:main' into main

07d15fd

Fix deserialization of kafka producer json config in the kafka-report…

c4c7e49

…er-plugin.

add changes doc

f0c0c7c

add com.google.common shade in the kafka-reporter-plugin.

e953bf6

wu-sheng added the rejected label May 31, 2023

wu-sheng closed this May 31, 2023

wu-sheng reopened this May 31, 2023

wu-sheng removed the rejected label May 31, 2023

wu-sheng added the bug Something isn't working label May 31, 2023

wu-sheng reviewed May 31, 2023

View reviewed changes

Fix com.google.common shade in the kafka-reporter-plugin.

1df8d64

gzlicanyi added 3 commits May 31, 2023 18:11

Change use gson TypeToken in the KafkaProducerManager.

c4ef0e5

Merge commit 'c04a71472' into sw8-main

4a6ec1c

delete kafka code

0ca303d

wu-sheng added this to the 8.16.0 milestone May 31, 2023

wu-sheng approved these changes May 31, 2023

View reviewed changes

wu-sheng merged commit dc602b7 into apache:main May 31, 2023

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

Fix deserialization of kafka producer json config in the kafka-report…

cd97aea

…er-plugin. (apache#542)

Fix deserialization of kafka producer json config in the kafka-reporter-plugin. #542

Fix deserialization of kafka producer json config in the kafka-reporter-plugin. #542

Uh oh!

Conversation

gzlicanyi commented May 31, 2023

Fix

Uh oh!

wu-sheng commented May 31, 2023

Uh oh!

gzlicanyi commented May 31, 2023

Uh oh!

wu-sheng commented May 31, 2023

Uh oh!

gzlicanyi commented May 31, 2023

Uh oh!

gzlicanyi commented May 31, 2023

Uh oh!

wu-sheng commented May 31, 2023

Uh oh!

gzlicanyi commented May 31, 2023

Uh oh!

wu-sheng May 31, 2023

Choose a reason for hiding this comment

Uh oh!

gzlicanyi May 31, 2023

Choose a reason for hiding this comment

Uh oh!

gzlicanyi May 31, 2023

Choose a reason for hiding this comment

Uh oh!

wu-sheng May 31, 2023

Choose a reason for hiding this comment

Uh oh!

gzlicanyi May 31, 2023

Choose a reason for hiding this comment

Uh oh!

wu-sheng May 31, 2023

Choose a reason for hiding this comment

Uh oh!

gzlicanyi May 31, 2023

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented May 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants