OpenTelemetry Middleware Proposal

[riandyrn]

Hello, @pkieltyka

First of all thank you so much for chi project! It has been a great help in my career as Go dev.

I would like to propose ownership of otelchi to go-chi org. It is basically middleware for sending traces to OpenTelemetry collector.

It also has been approved by OpenTelemetry community:

• https://github.com/open-telemetry/opentelemetry-go-contrib/pull/986#issuecomment-941280855
• https://github.com/open-telemetry/opentelemetry.io/pull/837

I'm just thinking maybe this project could be more useful if it is supported by the chi community itself. This is why I'm thinking to hand over its ownership to go-chi org.

I just hope it could be put under go-chi same like other middlewares (e.g cors & jwtauth) & being maintained by the community.

What do you think?

[pkieltyka]

hey @riandyrn this looks great :) thanks for the suggestion -- lets do it

A few things I'd like to do though..

1. We can transfer ownership to go-chi, and ill continue to give you collab writes to merge to the repo so you can help maintain it
2. Let's rename to github.com/go-chi/otel or github.com/go-chi/otelemetry
3. Rename folder example to _example

Separately, let me know if you'd be interested in discussing a Go development role at my company, https://horizon.io

[riandyrn]

ill continue to give you collab writes to merge to the repo so you can help maintain it

Thanks, it will be an honor to be able to work with chi community :)

Let's rename to github.com/go-chi/otel or github.com/go-chi/otelemetry

Sure, I believe github.com/go-chi/otelemetry will be more clear even though it's too long XD

Rename folder example to _example

Sure, I will make the change after the repo has been transferred. I will also:

1. Update CI Pipeline accordingly.
2. Update module name to be github.com/go-chi/otelemetry.
3. Update module name in OpenTelemetry registry.

Separately, let me know if you'd be interested in discussing a Go development role at my company, https://horizon.io

Yup, I'm totally interested :)

I will contact you soon through email you've shared in https://github.com/go-chi/chi/issues/696.

[riandyrn]

I've transferred the repo ownership to your account, @pkieltyka.

[VojtechVitek]

@riandyrn Would you consider changing the license to MIT, as the rest of go–chi projects? Or is there a particular reason for Apache instead? :)

This looks great. Thank you!

[riandyrn]

Hello, @VojtechVitek

Would you consider changing the license to MIT, as the rest of go–chi projects?

Yeah, this is actually one of the aspect I want to discuss.

I was using Apache 2.0 license because OpenTelemetry for Go is using it. So to contribute to it I need to also use the same license (see here).

But since I ended up hosting it by myself, I got confused (I'm total noob in OSS). 🤣

After looking into otelsql, I decided to keep the license as it is.

Personally I don't mind to change the license into MIT. But I'm not sure about the legality of it.

Maybe you know about this, @VojtechVitek?

[VojtechVitek]

If you're using upstream code directly in your repository (ie. if you copy pasted something or vendored something), then I think it needs to reference their original license.

However, if the middleware code is yours, it's all your decision :)

This is not a blocker of course, so I'll let you decide. But I thought MIT would be best fit to https://github.com/go-chi org, as all repos are MIT licensed.

[riandyrn]

If you're using upstream code directly in your repository (ie. if you copy pasted something or vendored something), then I think it needs to reference their original license.

Oh ok, I think it should be safe to change the license to MIT then. I just use the OpenTelemetry library.

Thanks for the explanation, @VojtechVitek. 👍🏻

Ok, should I apply all the required changes before the repo transfer is done, @pkieltyka?

I noticed you haven't approve the change ownership request for the repo.

Previously I thought the change would be better to be done after repo transfer so the community could also review it. 😅

[dp1140a]

Is this still alive? I give this whole thing a big +1 and am looking forward to using it?

[pkieltyka]

@dp1140a see https://github.com/go-chi/telemetry

[pkieltyka]

I'm still up for getting go-chi/otelemetry setup as well

[riandyrn]

I'm still up for getting go-chi/otelemetry setup as well

I'm glad that you decided to also develop the standard library for open telemetry, @pkieltyka. 🎉

I will close this issue in respect of this new project of yours. 👍🏻

Is this still alive? I give this whole thing a big +1 and am looking forward to using it?

@dp1140a, the riandyrn/otelchi library has improved greatly since I created this issue. I'm very lucky to get tons of help from many great people who interested in opentelemetry and runs otelchi library in their production system. 😃

Right now I'm no longer alone maintaining this otelchi library. It has been backed by my organization as well. So I can assure you that the code & maintenance quality will be good. So you can think riandyrn/otelchi as good secondary alternative for go-chi/otelemetry. 😃👍🏻

[pkieltyka]

hey @riandyrn the go-chi/telemetry project doesn't use opentelemetry, its a telemetry library built on prometheus directly.

I still think opentelemetry is the better path / route to go assuming (when) opentelemetry library is stable (and maybe that is already now)?

I think supporting both go-chi/telemtry (posted yesterday) and go-chi/otelemetry (your lib) is still a good idea, as both paths could be useful to implementors.

[inigohu]

Hi, why is this middleware necessary? There is an official otelhttp instrumentation in the opentelemetry-go-contrib repository supported by them. That could be included as chi middleware as well. or am I missing something?

[riandyrn]

Hello, @inigohu

Yeah, you are right it is not necessary for us to use ready to use middleware package (like otelchi) to integrate OpenTelemetry with Chi. You can use otelhttp as well.

However in the latest version of otelhttp (which is 0.42.0 at the time I wrote this message), the span it generates at least still missing following mandatory attributes from server span spec: http.target & http.route.

So if you want to use otelhttp you should use it like this:

// define router
chiRouter := chi.NewRouter()
chiRouter.Use(func(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		rCtx := chi.NewRouteContext()
		routePattern := ""
		if chiRouter.Match(rCtx, r.Method, r.URL.Path) {
			routePattern = rCtx.RoutePattern()
		}
		newHandler := otelhttp.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			span := trace.SpanFromContext(r.Context())
			span.SetAttributes(semconv.HTTPTarget(r.URL.String()), semconv.HTTPRoute(routePattern))
			h.ServeHTTP(w, r)
		}), routePattern)
		newHandler.ServeHTTP(w, r)
	})
})
// define handler
chiRouter.Handle("/users/{id:[0-9]+}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	id := chi.URLParam(r, "id")
	name := getUser(r.Context(), id)
	reply := fmt.Sprintf("user %s (id %s)\n", name, id)
	w.Write(([]byte)(reply))
}))
// serve router
_ = http.ListenAndServe(":8081", chiRouter)

I think it's much simpler if we define our router like this:

// define router
r := chi.NewRouter()
r.Use(otelchi.Middleware("my-server", otelchi.WithChiRoutes(r)))
// define handler
r.HandleFunc("/users/{id:[0-9]+}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	id := chi.URLParam(r, "id")
	name := getUser(r.Context(), id)
	reply := fmt.Sprintf("user %s (id %s)\n", name, id)
	w.Write(([]byte)(reply))
}))

// serve router
_ = http.ListenAndServe(":8080", r)

So yeah, it's not necessary for us to have ready to use middleware package and use otelhttp instead. But in my opinion it will be more prone to error. 😉

[inigohu]

Hi @riandyrn ,

Thank you for the clarification 👍🏻 . It is true that the http.target attribute has been removed but you could add the http.route with otelhttp.WithRouteTag.

[riandyrn]

It is true that the http.target attribute has been removed but you could add the http.route with otelhttp.WithRouteTag.

Yeah you are right, @inigohu. 👍🏻

However otelhttp.WithRouteTag only add http.route (we can see the details here), yet for some existing tools http.target might be still needed since it is previously set to be mandatory. This is why I prefer to add both http.target & http.route in one go.

Also please note that in the future most likely the OpenTelemetry specification team will add more mandatory attributes. So keeping this in one ready to use package will be much simpler for end user since they don't have to care much about all of these.

[sagikazarmark]

Here is my middleware using otelhttp:

r.Use(func(h http.Handler) http.Handler {
    return otelhttp.NewHandler(
        http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            h.ServeHTTP(w, r)

            routePattern := chi.RouteContext(r.Context()).RoutePattern()

            span := trace.SpanFromContext(r.Context())
            span.SetName(routePattern)
            span.SetAttributes(semconv.HTTPTarget(r.URL.String()), semconv.HTTPRoute(routePattern))

            labeler, ok := otelhttp.LabelerFromContext(r.Context())
            if ok {
                labeler.Add(semconv.HTTPRoute(routePattern))
            }
        }),
        "",
        otelhttp.WithMeterProvider(meterProvider),
    )
})

I don't know if populating any of the attributes or setting the name after running the handler is a problem, but so far it works.

[DavesBorges]

I don't know if populating any of the attributes or setting the name after running the handler is a problem, but so far it works.

@sagikazarmark I think that if in the handler you try to log the span id it won't be set yet so you couldn't correlate logs easily by span id