[Bug] The java-agent caused NATS to display '-ERR Unknown Protocol Operation'

[fly2F]

Search before asking

• I had searched in the issues and found no similar issues.

Apache SkyWalking Component

Java Agent (apache/skywalking-java)

What happened

Injected the sw8-x Header into a Message in NATS JetStream, but did not adjust the message, resulting in an abnormal message

What you expected to happen

No Error

How to reproduce

Skywaling: 9.5.0
Skywaling Java-Agent Version:
Nats Java SDK: 2.17.1

The Java Agent injected 'NATS/1.0' and 'sw8-x:0-1702370457015' into the NATS protocol

Error Log:

[42965] 2023/12/12 16:40:57.016125 [TRC] 172.20.31.146:37992 - cid:4737 - ->> [-ERR Unknown Protocol Operation]
====> [Liys] Parser error: Client parser ERROR, state=41, i=75: proto='"/1.0\r\nsw8-x:0-1702370457015\r\n\r\n+"...'
====> [Liys] Parser buf: PUB $JS.ACK.P1.bill_durable.2.75010.172953.1702362603906068441.2023 4
NATS/1.0
sw8-x:0-1702370457015

+ACK

====> [Liys] Parser msgBuf (len(c.msgBuf) >= c.pa.size)(0, 4) || (i-c.as+1 >= c.pa.size)(75, 71, 4):
[42965] 2023/12/12 16:40:57.016152 [ERR] 172.20.31.146:37992 - cid:4737 - Client parser ERROR, state=41, i=75: proto='"/1.0\r\nsw8-x:0-1702370457015\r\n\r\n+"...'
[42965] 2023/12/12 16:40:57.016154 [DBG] 172.20.31.146:37992 - cid:4737 - Client connection closed: Protocol Violation

Anything else

No response

Are you willing to submit a pull request to fix on your own?

• Yes I am willing to submit a pull request on my own!

Code of Conduct

• I agree to follow this project's Code of Conduct

[wu-sheng]

'NATS/1.0' is not injected by us, it is official part from NATS. sw-8 header is injected by NATS plugin.

The test cases we are using work well, ref https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/nats-2.14.x-2.15.x-scenario

And from I read from the plugin codes, the message has a header, I am not sure what is the issue.

https://github.com/apache/skywalking-java/blob/045c1e753f0f18a39566bd47dd6abf51ba4d2ccf/apm-sniffer/apm-sdk-plugin/nats-2.14.x-2.15.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/nats/client/NatsCommons.java#L67-L79

[pg-yang]

The challenge of intercepting the message constructor in the proper time is whether at that moment, we have a peer and it is the right time for starting the new exit span.

Yep, There is no proper time and uniform constructor for exit span, and building new may also face the same problem as the parameter NatsMessage of the instrumented method is iterative, replacing or rebuilding it is not easy.

Currently, there are 2 spans on producer side:

1. local span for recording operation of local block queue.
2. exit span for sending messages

We could remain span 1 as new exit span, and give up span 2, that just loses some details. WDYT?

[wu-sheng]

Local span would not be analyzed, which could not be a good practice.

Why don't use a new message with injected headers and all others from original message?

[pg-yang]

At the moment of sending messages', NatsMessage actually is a linked list, it has a field next to point next message.
To build a new linked list with new message, we must use reflection at least two times (one for reading, one for writing) as next is non-public.

https://github.com/nats-io/nats.java/blob/7c75122e59c1ca8344932a20fe0b46ad8a0b7687/src/main/java/io/nats/client/impl/MessageQueue.java#L207

[scottf]

The linked list thing isn't going to change. This is internal stuff, which has been optimized for speed, as was the pre-calculation of the message header.
If you can explain what is trying to be accomplished here, maybe I can help and we get changes directly into the client if they make sense but I think your are trying to be too granular with your time span. The fact that it takes a few milliseconds from when the message is put in the queue until the time it's copied to the socket is really irrelevant in the "time" it takes to send a message.

But fine, if that's your requirement, I am open to suggestions on how to change this. In the meantime, I've made a PR that copies the headers on message construction and makes them read-only, so you would get an error if you try to modify the copy (via getHeaders()) that is in the message. If you modify the original copy that you control and was used to make the message, it will not affect the message.

[wu-sheng]

@scottf The use case is, we are injecting header to propagate distibuted tracing context, as that, end user could link requests from produce side to consume side.

This is a standard operation for all tracing system, such as Zipkin. The different is, SkyWalking is using Java agent to do this, so, some methods of lib classes are changed dynamically.

Now, we have the message object, if you could make us possible to add new headers and re-calculate the length(or any other information), it should work well.