Skip to content

Conversation

@mar-kolya
Copy link
Contributor

So that spans inheriting from that client span have application
service name rather than 'kafka'

@mar-kolya mar-kolya requested a review from tylerbenson May 24, 2019 00:11
So that spans inheriting from that client span have application
service name rather than 'kafka'
@mar-kolya mar-kolya force-pushed the mar-kolya/kafka-client-service-name branch from 15323bb to 80a5cc6 Compare May 24, 2019 00:23
@mar-kolya mar-kolya requested a review from labbati May 24, 2019 00:34
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

LGTM! Just a thought though.

Assume that we have an app that only have a consumer and does not have a producer.

Wouldn't we loose visibility on the fact that such app depends on service Kafka? I may be missing something big here and what I wrote might make no sense.

While I do not object that the work done on the consumer callback can be considered part of the application service and not the kafka service, shouldn't we at least instrument the consumer constructor to keep track of the fact that we are at least depending on an external service? Unless I am missing this part from the code, we are not currently doing it.

@labbati labbati self-requested a review May 24, 2019 09:20
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

I wrongly approved it. Please see the comment above.

@mar-kolya
Copy link
Contributor Author

So in my humble opinion:

  • Kafka consumer span still has 'kafka' in some of its fields - so it is fairly obvious that it is coming from kafka when one looks at the span itself.
  • Delay in kafka consumer (or any queue consumer, for that matter) is usually not important - we do not care how long we wait for messages, we care long it takes to process them - so we mainly need consumer span to start off processing trace.
  • I think services would still be linked assuming producer has APM integration. If producer doesn't have an APM integration I would feel not much changes either.

I would agree that this may not be an ideal solution and it would be nice to have some way to change service name for children spans while not touching parent span - but so far I do not see a good way to do that,

@labbati
Copy link
Member

labbati commented May 24, 2019

I see the point and for me this is ok to merge.

My comment above was referred also to the fact that in the service map (as an example) you would not even see an arrow from application to a service named kafka. But this may be secondary.
So I stand by my approval then 😄

@mar-kolya mar-kolya merged commit bc37601 into master May 24, 2019
@mar-kolya mar-kolya deleted the mar-kolya/kafka-client-service-name branch May 24, 2019 14:51
@tylerbenson tylerbenson added this to the 0.29.0 milestone May 29, 2019
tylerbenson added a commit that referenced this pull request May 29, 2019
These are breaking changes that need more vetting.
tylerbenson added a commit that referenced this pull request May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants