Remove the TagLoggable tooling #225
Conversation
| // WithTagLoggable combines WithTaggable and logger.WithLoggable | ||
| // If the Loggable is a Taggable already (implements StatsTags), it will be used directly. | ||
| // If StatsTags doesn't exist, LogFields() will be used instead. | ||
| func WithTagLoggable(ctx context.Context, l logger.Loggable) context.Context { |
There was a problem hiding this comment.
I wonder if it makes sense to rewrite the implementation so it adds the Loggable. Then checks if its Taggable and add it. And remove the tagLoggable concept.
Just as a shorthand for doing
ctx = statsd.WithTaggable(ctx, shop)
ctx = logger.WithLoggable(ctx, shop)
There was a problem hiding this comment.
But I imagine we want to avoid doing this? Make it too easy to add whatever field to both and not think about it.
There was a problem hiding this comment.
unless what you mean is that fields implements loggable and taggle?
There was a problem hiding this comment.
As a user of this API, I very much prefer to write those two lines than wonder what WithTagLoggable does.
I agree that what you suggest is better than the current API, but I still think that KISS is better.
There was a problem hiding this comment.
@hurracrane I meant that the API will first do ctx = logger.WithLoggable(ctx, shop) Then it will try to cast it as Taggable and add it if so.
Basically avoid the fallback of directly adding LogFields as tags.
But I'm ok with us just removing the API
Remove the TagLoggable methods:
statsd.WithTagLogFields,statsd.WithTagLoggable,statsd.WatchingTagLoggable.This API is confusing and the caller can do the same thing by calling both
statsdandlogger.WithTagLoggableis even harmful since it will happily add log fields as metric tags.