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

Custom tags for spans of net/httpclient #2102

Closed
tkuenzle opened this issue Oct 14, 2022 · 6 comments
Closed

Custom tags for spans of net/httpclient #2102

tkuenzle opened this issue Oct 14, 2022 · 6 comments
Assignees
Labels
enhancement tracing tracing or opentracing related issue

Comments

@tkuenzle
Copy link

Is your feature request related to a problem? Please describe.
We are using the skipper/net/httpclient for quite a few services due to it's out-of-the-box support for opentelemetry. Recently a new requirement was raised, that we need to tag all outgoing client spans with specific tags, i.e. peer.service and peer.hostname. Currently, skipper/net/httpclient does only support very few tags, namely kind, component, http.url and http.method of which only component is configurable. We would like to be able to pass any additional tags that we need.

Describe the solution you would like
We would like to have a new option called sth like OpentracingTags or CustomOpentracingTags of type opentracing.Tags. These would be added to each span in addition to the previously supported ones.

Describe alternatives you've considered (optional)
Alternatively we could allow passing some kind of a hook as an option with the span as argument, so that the span could be modified in any way.

Would you like to work on it?
Yes

@szuecs
Copy link
Member

szuecs commented Oct 14, 2022

@tkuenzle I think it's fine to do the suggested feature with the suggested solution. CustomOpentracingTags sounds good to me.

@szuecs szuecs added enhancement tracing tracing or opentracing related issue labels Oct 14, 2022
@AlexanderYastrebov
Copy link
Member

I guess CustomOpentracingTags is a static set of tags.
We need to see how hard it is actually to use custom Tracer that overrides StartSpan and injects other tags there. It would allow to add dynamic tags.

AlexanderYastrebov added a commit that referenced this issue Oct 14, 2022
Updates #2102

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this issue Oct 14, 2022
Updates #2102

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov
Copy link
Member

We need to see how hard it is actually to use custom Tracer that overrides StartSpan and injects other tags there.

#2103

AlexanderYastrebov added a commit that referenced this issue Oct 14, 2022
Updates #2102

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Oct 14, 2022

Also we need to keep opentracing/specification#163 in mind before investing more into it, I've created #2104

@tkuenzle
Copy link
Author

Thanks for the POC @AlexanderYastrebov. Looks a lot easier than I assumed. It does still feel like a workaround and not like a nice solution though 😄

But especially in the light of opentracing being deprecated I guess it makes sense not to add more opentracing specific options 👍

AlexanderYastrebov added a commit that referenced this issue Oct 14, 2022
Updates #2102

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov
Copy link
Member

@tkuenzle Thank you, then I am closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement tracing tracing or opentracing related issue
Projects
None yet
Development

No branches or pull requests

3 participants