othttp: add WithSpanFormatter option #617
Conversation
marwan-at-work
requested review from
jmacd,
krnowak,
lizthegrey,
MrAlias,
paivagustavo,
rghetia and
tedsuo
as code owners
|
This looks good to me, though I'd like to see tests for the |
|
@Aneurysm9 I added a test case for As for other cases, I imagine a formatter that always returns |
plugin/othttp/handler.go
Outdated
|
|
||
| tracer trace.Tracer | ||
| props propagation.Propagators | ||
| spanStartOptions []trace.StartOption | ||
| readEvent bool | ||
| writeEvent bool | ||
| filters []Filter | ||
| spanFormatter func(*http.Request) string |
There was a problem hiding this comment.
I think that Handler should preserve the operation member and the span formatter should receive the operation name string as a parameter too, otherwise the operation parameter in the NewHandler function becomes useless if we pass a custom span formatter.
Also, shouldn't it be called spanNameFormatter?
There was a problem hiding this comment.
@krnowak the way I thought about it is that if a user passes "othttp.WithSpanFormatter", then they can assume "operation" is overwritten, but I will change that into your suggestion.
Also, spanNameFormatter sounds good, should we also change the option name from othttp.WithSpanFormatter into othttp.WithSpanNameFormatter?
There was a problem hiding this comment.
Also,
spanNameFormattersounds good, should we also change the option name fromothttp.WithSpanFormatterintoothttp.WithSpanNameFormatter?
Yes, please.
|
@krnowak I updated the PR with your suggestions, but I'm happy to backtrack or make further changes. Thanks |
Co-Authored-By: Rahul Patel <rghetia@yahoo.com>
Fixes #616