Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

SetTag should follow the same convention as LogFields #186

Open
MatthewDolan opened this issue May 18, 2018 · 2 comments
Open

SetTag should follow the same convention as LogFields #186

MatthewDolan opened this issue May 18, 2018 · 2 comments

Comments

@MatthewDolan
Copy link

Problem

I noticed that the log.Field object borrows from the convention set up by the zap logging library (https://github.com/uber-go/zap) to reduce allocations on logging by having typed functions (e.g. log.String(...) or log.Bool(...)). The SetTag(...) function does not follow that convention though.

Proposal

I would propose that SetTag takes a similar opentracing.Tag object that also reduces allocations.

For example:

span.SetTag(tag.String("hello", "world"))

Questions to address (if any)

There might need to be some path for rollout and backward compatibility.

@yurishkuro
Copy link
Member

I think it would've been nice if we did this originally, but at this point it will be a breaking change for SetTag API, which is hard to justify.

@MatthewDolan
Copy link
Author

MatthewDolan commented May 21, 2018

I agree that it doesn't quite make sense to make a user nonbackward-compatible change here.

Maybe adding a new API SetTagTyped. That's a terrible name, but that way it doesn't have impact on consumers. It would just require tracers to upgrade to expose the new API. I feel like changes for tracers are less disruptive than changes for consumers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants