Change otelecho to wrap otelhttp

[terev]

Summary

This pr changes the otelecho instrumentation to wrap the otelhttp instrumentation rather than implement it's own. This has many benefits in my opinion as the otelecho instrumentation wasn't doing anything novel. Wrapping the otelhttp instrumentation provides metrics in addition to a more standard trace middleware. I don't really expect this pr to be accepted as is because it kind of goes against the definition of an instrumentation package. Is there a different/better approach that should be taken here?

[codecov[bot]]

Codecov Report

Merging #2269 (4459e53) into main (d1faf5c) will decrease coverage by 0.5%. The diff coverage is 58.9%.

:exclamation: Current head 4459e53 differs from pull request most recent head bb53c82. Consider uploading reports for the commit bb53c82 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2269     +/-   ##
=======================================
- Coverage   74.3%   73.8%   -0.5%     
=======================================
  Files        144     145      +1     
  Lines       6563    6573     +10     
=======================================
- Hits        4877    4853     -24     
- Misses      1543    1578     +35     
+ Partials     143     142      -1     

Impacted Files	Coverage Δ
...thub.com/labstack/echo/otelecho/config_otelhttp.go	30.0% <30.0%> (ø)
...entation/github.com/labstack/echo/otelecho/echo.go	85.7% <82.6%> (-9.3%)	:arrow_down:
...tation/github.com/labstack/echo/otelecho/config.go	100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

[pellared]

I advise you to consider joining the SIG meeting to present the PR and ask for feedback. You can also ask for reviews on Slack. See: https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md

The comments I left are based on an issue that I was working on a long time ago. Maybe it is worth the revisit the topic. Nobody even "upvoted" my comments so maybe they are no longer invalid.

[pellared]

go: updates to go.mod needed; to update it:
	go mod tidy

[terev]

I ran precommit to fix the mod tidy issue.

[terev]

it looks like a linter failed because of a timeout on an unrelated module. anything else this pr needs to get in?

[dmathieu]

Unfortunately, the lint job is a bit flaky at the moment 😢 I reran it. But it seems the failure now is indeed related to your changes.

[pellared]

@open-telemetry/go-maintainers PTAL

[matoous]

@terev just out of curiosity, was this closed for lack of review or another reason?

[terev]

@matoous Yes mainly because it was not getting reviewed and possibly stale. Very happy to resubmit if others see value in it too lmk.

[matoous]

@terev I definitely do and would love to see this merged, also happy to contribute. Maybe @pellared could help get this through (please)?

[pellared]

I am reopening it and assigning to myself. However, I guess I will have to time to look at it in about a month 😬 @terev Can you make update the PR before tomorrow SIG meeting? I Then I can mention the PR during the SIG meeting.

Also I do not remember exactly how it is implemented but you can also explore how semconvutil is generated to reuse code between HTTP instrumentation libraries. This could potentially address this comment https://github.com/open-telemetry/opentelemetry-go-contrib/pull/2269#discussion_r915277852

PS. Sorry for inconvenience. I will do my best to help.

[terev]

Thanks for trying to get this some love for us but I just noticed this change in main https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/net/http/otelhttp/handler.go#L65 . This makes me think my change is no longer needed. Having the ability to create a net/http middleware directly allows us to wrap it and use it without need for a specific package. Once this is released we should be able to do:

mw := echo.WrapMiddleware(otelhttp.NewMiddleware("operation", opts...))
server.Use(mw)

@matoous does that seem reasonable to you?

[pellared]

@terev I think the "missing feature" of using otelhttp.NewMiddleware is that the route tags (like assert.Contains(t, attrs, attribute.String("http.route", "/user/:id"))) would be lost.

[terev]

@pellared Right that's a good point. maybe we need a variant of WithRouteTag that can be used as a middleware as well? I'd be happy to make that change.

[matoous]

@terev exactly as @pellared wrote. If you don't have time or capacity for it, please let me know, I would be happy to contribute.

[terev]

@matoous @pellared Submitted a new pull request here https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4017