Add WithRouteTagMiddleware function to otelhttp package #4017

terev · 2023-06-22T08:02:44Z

This new function simplifies usage of this utility by users of third party router packages.

CHANGELOG.md

instrumentation/net/http/otelhttp/handler.go

Co-authored-by: Damien Mathieu <42@dmathieu.com>

pellared · 2023-06-22T08:27:51Z

This new function simplifies usage of this utility within third party router packages.

Could you please provide examples? You can even consider adding "test examples".

EDIT: I think if we document how to use it with: gorilla/mux, echo, chi. We could even consider deprecating the otelmux, otelecho (or rewriting to use the same codebase).

instrumentation/net/http/otelhttp/handler.go

codecov · 2023-06-22T08:53:32Z

Codecov Report

Merging #4017 (efd7ba7) into main (9a582bd) will decrease coverage by 0.2%.
The diff coverage is 25.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4017     +/-   ##
=======================================
- Coverage   79.2%   79.0%   -0.2%     
=======================================
  Files        165     165             
  Lines      10318   10349     +31     
=======================================
+ Hits        8176    8182      +6     
- Misses      2006    2030     +24     
- Partials     136     137      +1

Impacted Files	Coverage Δ
...hub.com/emicklei/go-restful/otelrestful/restful.go	`90.5% <0.0%> (-9.5%)`	⬇️
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go	`79.1% <0.0%> (-4.4%)`	⬇️
...trumentation/github.com/gorilla/mux/otelmux/mux.go	`80.7% <0.0%> (-7.7%)`	⬇️
...entation/github.com/labstack/echo/otelecho/echo.go	`89.8% <0.0%> (-10.2%)`	⬇️
instrumentation/net/http/otelhttp/handler.go	`85.7% <90.0%> (+2.6%)`	⬆️

... and 2 files with indirect coverage changes

…to otelhttp-route-tag-middleware

pellared · 2023-06-22T09:19:06Z

instrumentation/net/http/otelhttp/go.sum

+go.opentelemetry.io/otel/sdk v1.16.0 h1:Z1Ok1YsijYL0CSJpHt4cS3wDDh7p572grzNrBMiMWgE=
+go.opentelemetry.io/otel/sdk v1.16.0/go.mod h1:tMsIuKXuuIWPBAOrH+eHtvhTL+SntFtXF9QD68aP6p4=


You have to move the test to /test subdirectory as the instrumentation libraries must not depend on the SDK.

terev · 2023-06-22T09:40:10Z

@pellared I've added one example so far and can add more in the morning. I added it to instrumentation/net/http/otelhttp/example/server/github.com/labstack/echo . It semanticly makes sense but may be tough to find. What do you think?

Add blurb to root example README

terev · 2023-06-25T20:13:37Z

@pellared I've added a bunch of examples to instrumentation/net/http/otelhttp/example/server. Probably not the best place so lmk if there's somewhere else they should be.

dmathieu · 2023-06-26T06:57:10Z

I think simpler, and testable examples would be better than this. What we need is a code sample, not a full-blown runnable app.

pellared

I am really on low time until Wednesday, but I did my best to put any feedback that can hopefully help.

pellared · 2023-06-26T07:17:44Z

instrumentation/net/http/otelhttp/example/server/github.com/labstack/echo/server.go

+	r.GET("/users/:id", func(c echo.Context) error {
+		id := c.Param("id")
+		name := getUser(c.Request().Context(), id)
+		return c.JSON(http.StatusOK, struct {
+			ID   string
+			Name string
+		}{
+			ID:   id,
+			Name: name,
+		})
+	}, echo.WrapMiddleware(otelhttp.WithRouteTagMiddleware("/users/:id")))


Would it not be possible to get rid of the "/users/:id" duplication?

I do not like requiring to add echo.WrapMiddleware(otelhttp.WithRouteTagMiddleware() for each registering handler.

I would prefer to have something like

r.Use(echo.WrapMiddleware(otelhttp.NewMiddleware("my-server"))) r.Use(OTelRouteTaggerMiddleware()) // or: r.Use(otelecho.RouteTaggerMiddleware()) r.GET("/users/:id", func(c echo.Context) error { id := c.Param("id") name := getUser(c.Request().Context(), id) return c.JSON(http.StatusOK, struct { ID string Name string }{ ID: id, Name: name, }) })

I hope something similar could be adopted for other libraries 😉

@pellared I can't come up with a way we can create a generic middleware for this since the route tag is usually router specific and not possible to retrieve in our middleware. If users want a catch all middleware I think each library would need to implement this. For example echo would need a middleware that uses the result of c.Path() as the route tag.

We could provide a utility function that adds a passed route tag as an attribute and changes the span name as discussed previously.

We could provide a utility function that adds a passed route tag as an attribute and changes the span name as discussed previously.

I think this may be more flexible and powerful. Writing a little middleware that would use this function should not be a big deal.

Added this utility function to otelhttp and route tagger middleware in 4 of the http router modules. Added a question about where to put the examples.

Can you update the examples? The existing example structure is good enough. We can refine it later once we have a consensus on the implementation part.

Add RouteTaggerMiddleware for most routers

terev · 2023-07-02T08:21:49Z

I think simpler, and testable examples would be better than this. What we need is a code sample, not a full-blown runnable app.

Not sure how to do this and actually add examples for the NewMiddlware function. If I add this into the root of the otelhttp module it'll add a bunch of extra dependencies. Should these examples be put in the example directory as testable examples? Even though they'll be examples for the example module?

dmathieu · 2023-07-03T07:49:08Z

Not sure how to do this and actually add examples for the NewMiddlware function. If I add this into the root of the otelhttp module it'll add a bunch of extra dependencies. Should these examples be put in the example directory as testable examples? Even though they'll be examples for the example module?

They can be put in the same directory, but with a different module name. See this example from gRPC: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/example_interceptor_test.go

terev · 2023-07-04T04:17:53Z

Not sure how to do this and actually add examples for the NewMiddlware function. If I add this into the root of the otelhttp module it'll add a bunch of extra dependencies. Should these examples be put in the example directory as testable examples? Even though they'll be examples for the example module?

They can be put in the same directory, but with a different module name. See this example from gRPC: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/example_interceptor_test.go

Won't that still require dependencies in the go.mod? I created a file called instrumentation/net/http/otelhttp/example_echo_middleware_test.go that is defined within the otelhttp_test package. Then running go mod tidy -v in that directory adds the dependencies required by the example to go.mod.

terev · 2023-07-25T20:08:02Z

It's still unclear to me where I should be putting the example tests. Maybe there is not really a good answer for this at the moment?

pellared · 2023-07-26T08:30:08Z

It's still unclear to me where I should be putting the example tests. Maybe there is not really a good answer for this at the moment?

For me it is good as it is for now. We can improve the examples later. You are correct here: #4017 (comment)

pellared · 2023-07-26T09:02:30Z

instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go

+// RouteTaggerMiddleware annotates the current span with the handler's route path.
+func RouteTaggerMiddleware() restful.FilterFunction {
+	return func(request *restful.Request, response *restful.Response, chain *restful.FilterChain) {
+		otelhttp.AnnotateSpanWithHTTPRoute(request.Request.Context(), request.SelectedRoutePath())
+		chain.ProcessFilter(request, response)
+	}
+}


We should not depend on otelhttp in other instrumentation libraries. These libraries are not stable and bumping in otelhttp can break otelgin users. The AnnotateSpanWithHTTPRoute implementation is so small that I would rather inline it in existing instrumentation packages.

I would prefer if this PR is only about otelhttp.

I would move the middleware implementations using AnnotateSpanWithHTTPRoute to examples under otelhttp. This would demonstrate how someone could use otelhttp e.g. instead of otelmux. Related comment: #4017 (comment)

terev · 2024-07-30T18:40:11Z

Does anyone have opinions on whether this should be closed? It seems like the third party instrumentation is due to be removed at some point so maybe this is obsolete?

dmathieu · 2024-07-31T07:28:23Z

With the routing enhancements in Go 1.22, and this PR being rather stale, I think we should close it.
Once all supported versions of Go have routing available, we can add that to otelhttp.

Note that while 1.22 added routing support, retrieving the pattern is only available from 1.23. So we'll have to wait for 1.22 to be EOL.
https://pkg.go.dev/net/http@master#Request.Pattern

Add WithRouteTagMiddleware function to otelhttp package

e1e07b8

terev requested a review from a team June 22, 2023 08:02

terev requested review from Aneurysm9 and dmathieu as code owners June 22, 2023 08:02

terev mentioned this pull request Jun 22, 2023

Change otelecho to wrap otelhttp #2269

Closed

dmathieu reviewed Jun 22, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

instrumentation/net/http/otelhttp/handler.go Show resolved Hide resolved

Update CHANGELOG.md

ebc787c

Co-authored-by: Damien Mathieu <42@dmathieu.com>

pellared reviewed Jun 22, 2023

View reviewed changes

instrumentation/net/http/otelhttp/handler.go Outdated Show resolved Hide resolved

terev added 2 commits June 22, 2023 04:55

TestWithRouteTagMiddleware

c11c774

Merge remote-tracking branch 'terev/otelhttp-route-tag-middleware' in…

0207940

…to otelhttp-route-tag-middleware

pellared mentioned this pull request Jun 22, 2023

otelhttp.WithRouteTag should update the span name #4018

Open

pellared reviewed Jun 22, 2023

View reviewed changes

terev added 2 commits June 22, 2023 05:33

Add example of instrumenting github.com/labstack/echo with otelhttp

9620c4d

Move WithRouteTagMiddleware test into sub test package

202313c

terev added 5 commits June 23, 2023 23:15

Add gorilla/mux otelhttp server example

5212653

Add gin-gonic/gin example

0421737

Add emicklei/go-restful example

4b0dee2

Add blurb to root example README

Ran precommit

addad4d

Add go-chi/example otelhttp server example

d7798c8

terev requested a review from pellared June 25, 2023 20:13

Fix name of stage in Dockerfile

b75e9c3

pellared reviewed Jun 26, 2023

View reviewed changes

hanyuancheung and others added 3 commits June 27, 2023 18:00

Merge branch 'main' into otelhttp-route-tag-middleware

66a3b1c

Merge branch 'main' into otelhttp-route-tag-middleware

9420b3a

Add utility function for annotating span with route

ecba8e0

Add RouteTaggerMiddleware for most routers

terev requested a review from hanyuancheung as a code owner July 2, 2023 08:19

hanyuancheung and others added 3 commits July 2, 2023 22:50

Merge branch 'main' into otelhttp-route-tag-middleware

7374d0b

Ran precommit

defa208

Fix tests

ecf518e

Merge branch 'main' into otelhttp-route-tag-middleware

efd7ba7

pellared reviewed Jul 26, 2023

View reviewed changes

This was referenced Aug 9, 2023

Deprecate otelmemcache, otelgocql #4164

Merged

Define an ownership model for packages that live in this repository #2834

Closed

pellared mentioned this pull request Sep 6, 2023

splunkchi: Support github.com/go-chi/chi/v5 signalfx/splunk-otel-go#316

Open

dmathieu closed this Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithRouteTagMiddleware function to otelhttp package #4017

Add WithRouteTagMiddleware function to otelhttp package #4017

terev commented Jun 22, 2023 •

edited

Loading

pellared commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

pellared Jun 22, 2023

terev commented Jun 22, 2023 •

edited

Loading

terev commented Jun 25, 2023

dmathieu commented Jun 26, 2023

pellared left a comment •

edited

Loading

pellared Jun 26, 2023 •

edited

Loading

terev Jun 28, 2023

pellared Jun 29, 2023

terev Jul 2, 2023

pellared Jul 26, 2023

terev commented Jul 2, 2023

dmathieu commented Jul 3, 2023 •

edited

Loading

terev commented Jul 4, 2023 •

edited

Loading

terev commented Jul 25, 2023

pellared commented Jul 26, 2023

pellared Jul 26, 2023 •

edited

Loading

terev commented Jul 30, 2024

dmathieu commented Jul 31, 2024

		go.opentelemetry.io/otel/sdk v1.16.0 h1:Z1Ok1YsijYL0CSJpHt4cS3wDDh7p572grzNrBMiMWgE=
		go.opentelemetry.io/otel/sdk v1.16.0/go.mod h1:tMsIuKXuuIWPBAOrH+eHtvhTL+SntFtXF9QD68aP6p4=

Add WithRouteTagMiddleware function to otelhttp package #4017

Add WithRouteTagMiddleware function to otelhttp package #4017

Conversation

terev commented Jun 22, 2023 • edited Loading

pellared commented Jun 22, 2023 • edited Loading

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

pellared Jun 22, 2023

Choose a reason for hiding this comment

terev commented Jun 22, 2023 • edited Loading

terev commented Jun 25, 2023

dmathieu commented Jun 26, 2023

pellared left a comment • edited Loading

Choose a reason for hiding this comment

pellared Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

terev Jun 28, 2023

Choose a reason for hiding this comment

pellared Jun 29, 2023

Choose a reason for hiding this comment

terev Jul 2, 2023

Choose a reason for hiding this comment

pellared Jul 26, 2023

Choose a reason for hiding this comment

terev commented Jul 2, 2023

dmathieu commented Jul 3, 2023 • edited Loading

terev commented Jul 4, 2023 • edited Loading

terev commented Jul 25, 2023

pellared commented Jul 26, 2023

pellared Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

terev commented Jul 30, 2024

dmathieu commented Jul 31, 2024

terev commented Jun 22, 2023 •

edited

Loading

pellared commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

terev commented Jun 22, 2023 •

edited

Loading

pellared left a comment •

edited

Loading

pellared Jun 26, 2023 •

edited

Loading

dmathieu commented Jul 3, 2023 •

edited

Loading

terev commented Jul 4, 2023 •

edited

Loading

pellared Jul 26, 2023 •

edited

Loading