Skip to content

Conversation

@mar-kolya
Copy link
Contributor

So spans parented by consumer span had reasonable service name

@mar-kolya mar-kolya requested review from labbati and tylerbenson 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.

Please see comment on #854
If the answer to that is 'no' then this can be merged for me.

/*
Use default service name. Common use-case here is to have consumer span parent
children spans in instrumented application. Since service name is inherited it makes
sense to default that to application service name rather than 'kafka'.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is rabbitmq?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks!

So spans parented by consumer span had reasonable service name
@mar-kolya mar-kolya force-pushed the mar-kolya/rabbitmq-client-service-name branch from 932fafb to a7271ed Compare May 24, 2019 13:06
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.

👍 for me

@mar-kolya mar-kolya merged commit 82f1a48 into master May 24, 2019
@mar-kolya mar-kolya deleted the mar-kolya/rabbitmq-client-service-name branch May 24, 2019 14:51
@brentm5
Copy link
Contributor

brentm5 commented May 24, 2019

I created #858 to help provide potentially a better path forward for the default rabbitmq consumer implementations. I have some concerns around what the above would do for metrics / alerts on the rabbitmq service. By doing this you would just move deliver methods to not show up as rabbitmq and instead the service.

@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.

5 participants