Add Pulsar configuration to determine the collection of message contents #657
Conversation
|
You should update pulsar test scenarios to cover the new parameters. |
| /** | ||
| * If set to true, the message contents of the Pulsar topic would be collected. | ||
| */ | ||
| public static boolean TRACE_MESSAGE_CONTENTS = true; |
There was a problem hiding this comment.
The default value should be false. The impact is clear for memory, network and database.
There was a problem hiding this comment.
agent.config set to false is not enough.
There was a problem hiding this comment.
Okay, I'll make some modifications and conduct unit testing. Thank you!
There was a problem hiding this comment.
@wu-sheng Shengge The relevant modifications have been submitted. Please check them when you have time. Thank you!
| @@ -73,6 +75,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr | |||
| Tags.MQ_BROKER.set(activeSpan, serviceUrl); | |||
| Tags.MQ_TOPIC.set(activeSpan, consumer.getTopic()); | |||
| activeSpan.setPeer(serviceUrl); | |||
| if (Pulsar.TRACE_MESSAGE_CONTENTS) { | |||
| Tags.MQ_BODY.set(activeSpan, StringUtil.cut(new String(msg.getData()), Pulsar.MESSAGE_CONTENTS_MAX_LENGTH)); | |||
There was a problem hiding this comment.
msg.getData() is a byte[]. Is here OK to use the default charset?
There was a problem hiding this comment.
We used the default encoding for a long time without any issues, but we are using Java and are unsure if other languages can execute properly. This time, I added the UTF-8 character set and it ran normally in our environment
|
Please update this test, https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/pulsar-scenario Plugin test could run locally, https://skywalking.apache.org/docs/skywalking-java/v9.1.0/en/setup/service-agent/java-agent/plugin-test/ You should activate this new config and verify new tags. |
|
There are some issues with the environment of my computer. I will submit it after I run it. Thanks! |
|
This test (https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/pulsar-scenario) has passed (Scenario[pulsar-scenario-2.2.1] passed!) in the local environment and has been submitted. |
| @@ -97,6 +99,7 @@ segmentItems: | |||
| - { key: transmission.latency, value: not blank } | |||
| - { key: mq.broker, value: not blank } | |||
| - { key: mq.topic, value: test } | |||
| - { key: mq.body, value: "1" } | |||
There was a problem hiding this comment.
This is how the message is built,
producer.newMessage().key("testKey").value(Integer.toString(1).getBytes()).property("TEST", "TEST").send();But you only set 1 as body? Could you explain?
There was a problem hiding this comment.
This 1 was set by the previous submitter, and I only tested the corresponding expected value based on this value. If need test message bodies in other formats, I can modify and resubmit them.
There was a problem hiding this comment.
But there is not only value. There are a key and properties. aren't those message body too?
|
Could you explain what you changed? Personally, I am frustrated about the communications between us. I have to read codes, understand codes, but you never relies words. |
|
Actually, I only added a dynamic configuration item to dynamically collect the message contents of Pulsar. I changed "mq.body" to "mq.message_contents" and have already submitted it. This can more clearly reflect the content I added and the expected results. |
|
My question is key/value, why is the key not collected? |
|
The key is Pulsar's strategy for message routing, and its relationship with actual messages can be inclusive or independent. I just wanted to collect actual messages, so I didn't consider this key. Perhaps my consideration was a bit lacking. |
|
Is there pulsar doc indicating key is not a part of message? I am doubt about that. |
|
@tisonkun What is the message concept in the Pulsar? |
|
Misunderstand my meaning, key must be passed together with the content of the message body, but the collection content I want to add does not take into account key, I will add it sometime today. |
|
The messages in Pulsar include Value/data payload, Key, Properties, etc. My initial idea was just to collect Values. |
|
That doesn't make sense from my understanding, unless this is from Pulsar officially. |
|
OK, it should be my lack of understanding. I'll readjust it again. I'm really sorry for wasting your time. |
|
For next time, please write the proposal that you design first. It would help me align with you. |
|
Ok, thank you very much! |
|
Any update about this/ |
|
I was sick last week. I have time to deal with it this week. |
|
Is this still on going? |
|
Shengge, After careful consideration later, maybe this feature is not very necessary, and the demand for simply collecting message content is probably not too much, so I want to give up this pull request. |
|
Closing upon request. |
If this is non-trivial feature, paste the links/URLs to the design doc.
Update the documentation to include this new feature.
Tests(including UT, IT, E2E) are added to verify the new feature.
If it's UI related, attach the screenshots below.
Modify the unit test to verify that the feature works.
The configuration has been verified in practice.
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the
CHANGESlog.