Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing spans in gorillamux instrumentation #86

Merged
merged 27 commits into from
May 12, 2023

Conversation

JamieDanielson
Copy link
Member

@JamieDanielson JamieDanielson commented Apr 19, 2023

Which problem is this PR solving?

Short description of the changes

The first commit updates the expected output of a trace for nethttp instrumentation, removing the empty second span. The next set of commits adjusts the http server instrumentation, ultimately just adjusting one of the changes in #34 that used an array of two function names for ServeHttp and keeping only the newer "net/http.HandlerFunc.ServeHTTP".

  • Another commit updates the expected output of a trace for gorillamux instrumentation, updating the empty gorilla/mux span to include the http info.
  • And finally, the gorillamux instrumentation was updated to match the nethttp/server changes made in Fix net/http instrumentation for register-based ABI #34
  • Updated per PR feedback: Method Length has been set to 7 instead of 6 to include items like OPTIONS. This has been adjusted for gin-gonic and nethttp server instrumentation as well.

I'm not really sure what the trace for gorillamux should look like; the same info exists for two spans, but one is named for net/http and is for gorillamux. This doesn't address that. The trace was already emitting two spans, with the gorillamux span being empty - and this PR populates the gorillamux span.

Note: This has been updated as several of the changes made were ported into #82 . Since then, the gorilla/mux spans were empty. This PR still has the effect of populating the gorilla/mux spans.

@JamieDanielson JamieDanielson changed the title wip: fix empty span in net/http server instrumentation fix empty span in net/http server instrumentation Apr 19, 2023
@JamieDanielson JamieDanielson marked this pull request as ready for review April 19, 2023 20:31
@JamieDanielson JamieDanielson requested a review from a team April 19, 2023 20:31
@JamieDanielson JamieDanielson marked this pull request as draft April 19, 2023 21:00
@JamieDanielson JamieDanielson changed the title fix empty span in net/http server instrumentation wip: fix empty span in net/http server instrumentation Apr 19, 2023
@JamieDanielson JamieDanielson changed the title wip: fix empty span in net/http server instrumentation fix empty spans in net/http server and gorillamux instrumentation Apr 20, 2023
@JamieDanielson JamieDanielson marked this pull request as ready for review April 20, 2023 20:37
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting!

@JamieDanielson
Copy link
Member Author

Just a note, I'd been holding off on any further changes here to help get #92 across the line first with minimal merge conflicts there. Some of these changes are in #82 now so I'll wait on that one as well and then there may be nothing left on this one. If anyone thinks this approach should change let me know.

@JamieDanielson JamieDanielson changed the title fix empty spans in net/http server and gorillamux instrumentation [fix] fix missing spans in gorillamux instrumentation May 5, 2023
@JamieDanielson JamieDanielson changed the title [fix] fix missing spans in gorillamux instrumentation Fix missing spans in gorillamux instrumentation May 5, 2023
@JamieDanielson
Copy link
Member Author

This is ready for re-review. For reference, running make fixture-gorillamux without replacing the trace/span IDs with "xxxxx" results in different trace/span IDs which is expected, so it's not a true duplicate of the net/http span. The extra net/http span is worth looking at but may be best addressed in a separate PR, and will be fixed if we implement consolidated http instrumentation referenced in #101

          "scope": {
            "name": "github.com/gorilla/mux"
          },
...
              "name": "/users/foo",
              "parentSpanId": "",
              "spanId": "a965bd2e4dec00c4",
              "status": {},
              "traceId": "5a5dc1cc0de7798f1d349248975d35db"
...
          "scope": {
            "name": "net/http"
          },
...
              "name": "/users/foo",
              "parentSpanId": "",
              "spanId": "ad51aa0cebcf8d6b",
              "status": {},
              "traceId": "a4a1761ce09da7ca6cc54d7d4d25611b"

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/instrumentors/bpf/github.com/gorilla/mux/probe.go Outdated Show resolved Hide resolved
@JamieDanielson JamieDanielson requested a review from MrAlias May 9, 2023 20:56
@MrAlias MrAlias merged commit 5bd910d into open-telemetry:main May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

empty spans are generated by auto-instrumentation
7 participants