[ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp

[StyleTang]

Make sure set the target branch to develop

What is the purpose of the change

TraceDataEncoder add SubAfter trace bean timestamp
Mentioned in PR-744 , the timeStamp and consumeGroup have been deleted by this commit.

(It is fine for deleting SubAfter consumeGroup because we can find consume group from SubBefore traceContext by messageId and requestId)

I use subBefore timestamp+costTime instead of subAfter timestamp, but it is not reasonable.

Brief changelog

TraceDataEncoder add SubAfter trace bean timestamp

Verifying this change

Unit Tests succeed

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

• Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
• Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
• Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

[coveralls]

Coverage increased (+0.007%) to 54.021% when pulling e8a0152 on StyleTang:trace-ui into 589e87c on apache:develop.

[vongosling]

Very responsible for this issue closed, thanks ~

[yuz10]

The code here is used for compatible reason, if the message is produced by old versions of producer, there will be group name.

[StyleTang]

Get it.
I removed it because I thought this value is useless, maybe add the branch line.length >= 9 back is a better way.

        // compatible with the old version
        if (line.length >= 8) {
            subAfterContext.setTimeStamp(Long.parseLong(line[7]));
        }
        if (line.length >= 9) {
            subAfterContext.setGroupName(Long.parseLong(line[8]));
        }

[duhenglucky]

It is not recommended to modify this format in the current state. The previous version upgrade brought many compatibility issues.

[StyleTang]

@yuz10 @duhenglucky Thanks for your comment.

For subAfterContext, in the previous version, it was already defined line[7] is TimeStamp and line[8] is GroupName, it means for compatible reason in new revision we should still follow this protocol.

The subAfter GroupName is duplicated with subBeofre, so based on performance considerations(I guess), it was removed it by https://github.com/apache/rocketmq/pull/3005/files

We have several ways to make it compatible.

Option1 ( Prefer )

Just follow the previous version subAfter protocol.

( line[7] is TimeStamp and line[8] is GroupName )

Props

• The protocol remains consistent, easy to maintain

Cons

• In the future revision should add line[8] group name back, even if it is useless

Option2

If line[8] GroupName was never used, just remove it as if it not exist.

Props

• Performance friendly (can remove the duplicated groupName)

Cons

• The protocol is changed (Not a good way)
• Compatible logic is complicated, old version line[8] is group name while new revision may be not. (The new revision content length should not be 9, if it is 9, we can’t tell what the value it is)

Do you have any suggestions for this?

[yuz10]

@StyleTang I prefer just add 2 back and keep the same with 4.9.0.

[vongosling]

Go ahead~ Designed well is what we have advocated. The previous disturbing is the obvious lack of this point.

[vongosling]

For this problem, Who said before and after must be atomic exit?

[StyleTang]

@yuz10
Already add them back.

@vongosling
You are right. There is a small probability before and after trace not atomic exist.
If consumeMessageBefore has already been executed and the trace message has been sent by localDispatcher, then the system crashed, subAfter trace may be lost.

org.apache.rocketmq.client.impl.consumer.ConsumeMessageConcurrentlyService.ConsumeRequest#run

      if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) {
          consumeMessageContext = new ConsumeMessageContext();
  ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookBefore(consumeMessageContext);
      }
     try {
          status = listener.consumeMessage(Collections.unmodifiableList(msgs), context);
      } catch (Throwable e) {
      }
  
      // System crash happen 
      if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) {           ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookAfter(consumeMessageContext);
      }

[vongosling]

@duhengforever @duhenglucky pls help to look at this issue, especially compatibility backword:-0

[StyleTang]

I think it is necessary for us to add Timestamp and GroupName back.
The reason is that send SubBefore and SubAfter message trace is not a atomic operation, we may only get SubBefore or SubAfter trace, If SubAfter trace don't have GroupName we don't know which subscription group consumed it.

[vongosling]

Yes, I would like to hear any compatible issues that resolved from the opinion of the other @duhengforever @zongtanghu I think this pr apache/rocketmq-externals#769 would resolve the problem from the dashboard viewpoint.