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.

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

Update Kafka messaging example #1799

Closed

Conversation

trask
Copy link
Member

@trask trask commented Jul 7, 2021

I think(?) this was just an oversight in the example, and "Span Prod2" should parent "Span Proc1" (and no need for a link to "Span Prod1").

@trask trask requested review from a team July 7, 2021 01:47
Comment on lines -261 to +262
| Parent | | Span Prod1 | Span Rcv1 | | Span Prod2 |
| Links | | | | Span Prod1 | |
| Parent | | Span Prod1 | Span Rcv1 | Span Proc1 | Span Prod2 |
| Links | | | | | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already made a comment about this in the original PR at #1027 (comment) where @kenfinnigan then explained why a parent/child relationship might not always be possible or applicable.
Maybe we understand this problem better now that the first Kafka instrumentations based on these semantic conventions were already developed.
CC @anuraaga who might have an opinion here.

Copy link
Member

@kenfinnigan kenfinnigan Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My views have evolved somewhat, but I think it's also colored by #65 not being resolved, and how the spans look when visualized.

While I still think it makes sense for producing a new message to have no parent on the "process" of the existing one, it really messes with the visualization aspect as it gives the impression, as I mentioned in #1085 (comment), that the production of a new message is a child of the incoming span.

I'm ok with adjusting things as it makes the visualization clearer. Maybe it is revisited when there is a concept of "follows from".

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Jul 7, 2021
@kenfinnigan
Copy link
Member

kenfinnigan commented Jul 7, 2021

Thanks @trask for doing this.

From re-reading it, I think my preference would be for the parent to be "Span Rcv1" and not "Span Proc1". The reason is that "Span Proc1" isn't the bit that produces the new message, it occurs in frameworks after the processing of the message is complete. So they're sequential operations, but not directly called one to the other.

@trask
Copy link
Member Author

trask commented Jul 7, 2021

"Span Proc1" isn't the bit that produces the new message, it occurs in frameworks after the processing of the message is complete

ah, I was assuming "Span Proc1" represents the entire processing of the message, e.g. a @JmsListener

@kenfinnigan
Copy link
Member

I think it still represents the entirety of the processing for a specific message, as in @JmsListener.

However, the creation of another message out after processing a message, at least in reactive messaging cases we have with Kafka and AMQP, is a separate step that runs once the message processing is complete.

In essence, we create a reactive stream of "receive the incoming message" -> "process message" -> "create new outgoing message", with each step a separate component in the reactive stream

@trask
Copy link
Member Author

trask commented Jul 7, 2021

@kenfinnigan just to clarify, it sounds like you agree with this change for non-reactive messaging flows? If so, maybe you could propose a separate example for reactive messaging flows?

@kenfinnigan
Copy link
Member

The proposed change is to the Kafka example, which would be applicable to reactive messaging flows.

I don't remember enough about JMS to say whether what I think fits for the Kafka example is applicable to JMS and a Broker situation, or if a separate one is needed.

@trask
Copy link
Member Author

trask commented Jul 7, 2021

oh, sorry, I think I confused things by mentioning @JmsListener above.

how would you model @KafkaListener?

@kenfinnigan
Copy link
Member

I'm not super familiar with @KafkaListener, but from an initial look it seems @KafkaListener on its own would be the same as the Kafka example up to "Span Proc1" as the listener on its own just processes a message.

If a method had @KafkaListener and @SendTo present, it would be exactly the same as the Kafka example in that the method processing the message will return another message, but it's the framework that actually sends the new outgoing message to Kafka, and not the method processing the incoming message.

I might be wrong, but that's what it appears would be the case with an initial look

@trask
Copy link
Member Author

trask commented Jul 7, 2021

I see, I was assuming the example was depicting the processing code sending the downstream message directly, as opposed to returning the message to be sent by the framework

@kenfinnigan
Copy link
Member

@trask you mean the example for Apache Kafka? If so, I think I was the one that originally added it and it was based on how messaging is usually handled with reactive streams in that the communication with Kafka is handled by the framework.

After reviewing some traces the other day I realized what I'd proposed as the convention was a bit off, and should be adjusted

@trask
Copy link
Member Author

trask commented Jul 8, 2021

@trask you mean the example for Apache Kafka? If so, I think I was the one that originally added it and it was based on how messaging is usually handled with reactive streams in that the communication with Kafka is handled by the framework.

thx, makes sense, I'll close this PR then since that was not my interpretation of the example

@trask trask closed this Jul 8, 2021
@trask trask deleted the update-kafka-messaging-example branch July 8, 2021 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants