Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TracingChannelInterceptor: simple refactoring #1948

Merged
merged 3 commits into from May 25, 2021
Merged

TracingChannelInterceptor: simple refactoring #1948

merged 3 commits into from May 25, 2021

Conversation

artembilan
Copy link
Contributor

No description provided.

@artembilan
Copy link
Contributor Author

This PR contains a simple and short refactoring of TracingChannelInterceptor code style.

Feel free to merge or reject.

The purpose of this PR is more about starting a discussion and possible raising some issues here or there to address later on or immediately.

As it turns out the logic of this component is complex enough.

  1. We probably could have a separate interceptor for Spring WebSocket support - all the Spring Integration MessageChannel implementation are an AbstractMessageChannel.
  2. We probably can have a separate interceptor for Spring Cloud Stream. (Or we have already but some other way).
  3. If we still are about a ChannelInterceptor impl for Spring Integration, then it is better to look into a ThreadStatePropagationChannelInterceptor. The SecurityContextPropagationChannelInterceptor is the best sample to follow: https://docs.spring.io/spring-integration/docs/current/reference/html/security.html#security-context-propagation
  4. It is not OK to clean up a thread local in the afterReceiveCompletion(). Well, better to say it is pointless to do thread state manipulation with postReceive() and afterReceiveCompletion() since the state is cleaned up when message is produced from the receive(). On one hand we can think to not modify thread local over there at all, or mention in the docs how to clean it up from end-user perspective. See the mentioned Security concern in Spring Integration docs.
  5. private static final String REMOTE_SERVICE_NAME = "broker"; and its JavaDocs. Or this one is about WebSockets support or it is really a general concept to represent an endpoint on the consumer and produce side. (Correct me if I'm wrong). For tracking this purpose I definitely can suggest something similar what we do with MessageHistory in Spring Integration: https://docs.spring.io/spring-integration/docs/current/reference/html/system-management.html#message-history. And how we deal with the TrackableComponent. This way not only MessageChannel abstraction would be involved in the tracing, but also all the MessageHandlerSupport, MessageProducerSupport, MessagingGatewaySupport implementations, plus a SourcePollingChannelAdapter. This way we would have tracing aligned with the MessageHistory and also with what we do when deal with metrics.

I probably miss something else: feel free to point me out what to pay close attention!

We can live for now with what we have so far or let me know if you want we to remove that thread state manipulation for the receive() part.

Thank you for letting me know what is going on in tracing around Spring Integration!

@marcingrzejszczak
Copy link
Contributor

The changes look nice!

We probably could have a separate interceptor for Spring WebSocket support - all the Spring Integration MessageChannel implementation are an AbstractMessageChannel.

You're suggesting to remove the code from this interceptor and move the whole code to a separate one?

If we still are about a ChannelInterceptor impl for Spring Integration, then it is better to look into a ThreadStatePropagationChannelInterceptor. The SecurityContextPropagationChannelInterceptor is the best sample to follow: https://docs.spring.io/spring-integration/docs/current/reference/html/security.html#security-context-propagation

Ok I'll look into that code.

It is not OK to clean up a thread local in the afterReceiveCompletion(). Well, better to say it is pointless to do thread state manipulation with postReceive() and afterReceiveCompletion() since the state is cleaned up when message is produced from the receive(). On one hand we can think to not modify thread local over there at all, or mention in the docs how to clean it up from end-user perspective. See the mentioned Security concern in Spring Integration docs.

Sure, please update the code then.

private static final String REMOTE_SERVICE_NAME = "broker"; and its JavaDocs. Or this one is about WebSockets support or it is really a general concept to represent an endpoint on the consumer and produce side. (Correct me if I'm wrong).

Correct, however in between we have a broker so we will see a component in Wavefront / Zipkin called broker.

For tracking this purpose I definitely can suggest something similar what we do with MessageHistory in Spring Integration: https://docs.spring.io/spring-integration/docs/current/reference/html/system-management.html#message-history. And how we deal with the TrackableComponent. This way not only MessageChannel abstraction would be involved in the tracing, but also all the MessageHandlerSupport, MessageProducerSupport, MessagingGatewaySupport implementations, plus a SourcePollingChannelAdapter. This way we would have tracing aligned with the MessageHistory and also with what we do when deal with metrics.

I'm all for such changes. If this is a new feature though then we should do it in 3.1.x branch. How would you like to proceed with that?

@artembilan
Copy link
Contributor Author

Thanks Marcin for quick turn around!

Sure, please update the code then.

Sure, will do soon! And this is a bare minimum we can do for now to keep the code backward compatible.
I still have some doubts about copying/pasting headers, but I guess there was a reason to do that since WebSocket stuff is involved in that logic. My best guess to be able to keep an existing id and timestamp headers.

If this is a new feature though then we should do it in 3.1.x branch.

Sure! Let me see what I can do over there. Probably somewhere next week after releases.

@artembilan
* More clean up in the TracingChannelInterceptor
5fd6526 
 - Move Spring Cloud Stream classes logic into `static` methods
to avoid eager load for those classes which are not present on CP
- Remove `emptyMessage()` since the contract for `ChannelInterceptor`
never accept a `null`
- Remove thread state manipulation from the `postReceive()` since
no one takes care about thread local afterwards
- Remove `afterReceiveCompletion()` in favor of its `default` impl
in the interface: we don't do thread local manipulation for `receive()`
since `afterReceiveCompletion()` is called *before* the message is really
returned to the target subscriber on the channel
@artembilan
* Restore span manipulation for receive().
58c8e05 
Even if we don't need a thread local store, we still
need to call `span.end()` and set an exception tag
to it if any.
So, just reuse an existing API which includes thread local.
Even if we don't need it logically, there might be some
other interceptors in between which would like to take a
span from thread local do something even on that "void"
`receive()` operation
@marcingrzejszczak
Copy link
Contributor

Is it ready to be merged? :)

@artembilan
Copy link
Contributor Author

Sure thing, Marcin!
Sorry for not making myself clear when we discussed this in person.

Yes, it can be merged already if you don't have anything else I should pay attention to.

Thanks

@marcingrzejszczak
Copy link
Contributor

Great. I wonder about one thing. How about we merge this not to main but to 3.1.x branch? Since the current main should pretty much contain only bug fixes. WDYT?

@artembilan
Copy link
Contributor Author

Sure! However I think those classpath checks I have fixed we can treat as bug and therefore back-port 😄 .
But if this is still to stressful, I'm OK just have a fix only for the next version.

@marcingrzejszczak
Copy link
Contributor

Ah no, if it's a bug fix then no problem. I'll merge this.

@marcingrzejszczak marcingrzejszczak merged commit fa1e0b8 into spring-cloud:main May 25, 2021
@marcingrzejszczak marcingrzejszczak added this to In progress in 2020.0.3 via automation May 25, 2021
@marcingrzejszczak marcingrzejszczak added this to the 3.0.3 milestone May 25, 2021
@spencergibb spencergibb moved this from In progress to Done in 2020.0.3 May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcingrzejszczak marcingrzejszczak Awaiting requested review from marcingrzejszczak

Assignees
No one assigned
Labels
bug
Projects
No open projects
2020.0.3
  
Done
Milestone
3.0.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@artembilan @marcingrzejszczak @spring-cloud-issues