Add otelhttp.NewMiddleware #2964
Conversation
alnr
requested review from
jmacd,
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
|
|
Aneurysm9
added
enhancement
|
@alnr thanks for the contribution, please sign the CLA so these changes can be considered. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2964 +/- ##
=====================================
Coverage 79.3% 79.3%
=====================================
Files 165 165
Lines 10422 10426 +4
=====================================
+ Hits 8274 8278 +4
Misses 2012 2012
Partials 136 136
|
|
Hey. I'll do the paperwork early next year, sorry for the delay. Have a great new year! |
|
I also would like this change, will this be followed up? |
|
It looks like @alnr never signed the CLA. So we can't use his work there. |
|
I'll see if he answer today, if not I'll submit a followup tomorrow. |
|
Sorry, this got lost among the paperwork. CLA signed. |
|
|
||
| // / NewMiddleware returns a tracing middleware from the given operation name and | ||
| // options. | ||
| func NewMiddleware(operation string, opts ...Option) *Middleware { |
There was a problem hiding this comment.
The middlewares I usually see are like the one for gorilla mux, in the form:
type MiddlewareFunc func(http.Handler) http.Handleris this different in chi? I think most http middleware library accepts this form.
There was a problem hiding this comment.
func(http.Handler) http.Handler is most common, I agree. It's simple enough to write adapters for other uses.
I've pushed another commit, which gets rid of the otelhttp.Handler type. That's not strictly backwards compatible. Is that OK?
There was a problem hiding this comment.
I've pushed another commit, which gets rid of the otelhttp.Handler type. That's not strictly backwards compatible. Is that OK?
I think it is fine as the type does not make any sense. However, in future I would rather propose to do such changes in a separate PR.
|
Rebased and fixed lint (I hope?) |
pellared
removed
the
blocked: CLA
|
Gentle bump for review or merge :) |
|
@alnr Can you please update the branch (and resolve the conflicts)? |
…TTP routers and middleware chainers The key difference between the existing otelhttp.NewHandler and otelhttp.NewMiddleware is that the middleware takes the `next` handler as an argument after construction, wheres NewHandler works by wrapping one specific http.Handler at construction time.
done |
For better compatiblity with third-party HTTP routers and middleware chainers (e.g. github.com/urfave/negroni or github.com/go-chi/chi).
The key difference between the existing
otelhttp.Handlerand the newotelhttp.Middlewareis thatMiddlewaretakes thenexthandler as an argument after construction, wheres the existingHandlerworks by wrapping one specific http.Handler at construction time. The existing interfaces are unchanged.Is there interest in principle in merging this? If yes I'll deal with the CLA etc.