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

feat: Support http.route attribute for gin and chi #1359

Closed
wants to merge 4 commits into from

Conversation

iBluemind
Copy link

The name of the HTTP span must be low-cardinality.
To achieve this, the http.route attribute can be included in the span's name.

Semantic conventions for HTTP spans

HTTP span names SHOULD be {method} {target} if there is a (low-cardinality) target available.

The {target} SHOULD be one of the following:

However, most Go-based web frameworks use different methods for parsing routing paths.
It's not feasible to handle all cases with just an eBPF probe for net/http.

Therefore I submit a patch that adds the eBPF probe support for gin and chi frameworks.

@iBluemind iBluemind force-pushed the feat/http-route branch 2 times, most recently from 6528c2f to 35ef7a1 Compare December 2, 2024 15:49
@iBluemind iBluemind marked this pull request as ready for review December 2, 2024 15:50
@iBluemind iBluemind requested a review from a team as a code owner December 2, 2024 15:50
@iBluemind iBluemind force-pushed the feat/http-route branch 2 times, most recently from d7d64e4 to afa4e69 Compare December 2, 2024 16:02
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

I am not in favor of accepting these changes:

@iBluemind
Copy link
Author

@MrAlias Thank you for your prompt feedback. I'm aware of the #780 in question.
However, most frameworks, including Gin, do not use the ServeMux pattern. So I think there are limitations to using only net/http probes.

@MrAlias
Copy link
Contributor

MrAlias commented Dec 2, 2024

However, most frameworks, including Gin, do not use the ServeMux pattern.

I don't follow, how are we able to produce telemetry for gin as exemplified in our end-to-end tests?

@iBluemind
Copy link
Author

iBluemind commented Dec 2, 2024

@MrAlias The e2e test is written like this line.

r.GET("/hello-gin/:id", hello)

It passed by this line.

@test "server :: includes http.route attribute" {
  result=$(server_span_attributes_for ${SCOPE} | jq "select(.key == \"http.route\").value.stringValue")
  assert_equal "$result" '"/hello-gin/:id"'
}

I ran the e2e test by this workflow.

Below is the backtrace from Delve attached to our nethttp example. (pat is filled)

> net/http.(*conn).serve() /opt/homebrew/Cellar/go/1.23.1/libexec/src/net/http/server.go:2093 (PC: 0x10077c16c)
Warning: debugging optimized function
Values returned:

  2088:			// in parallel even if their responses need to be serialized.
  2089:			// But we're not going to implement HTTP pipelining because it
  2090:			// was never deployed in the wild and the answer is HTTP/2.
  2091:			inFlightResponse = w
  2092:			serverHandler{c.server}.ServeHTTP(w, w.req)
=>2093:			inFlightResponse = nil
  2094:			w.cancelCtx()
  2095:			if c.hijacked() {
  2096:				return
  2097:			}
  2098:			w.finishRequest()
(dlv) p req
("*net/http.Request")(0x140000b4000)
*net/http.Request {
	Method: "GET",
	URL: *net/url.URL {
		Scheme: "",
		Opaque: "",
		User: *net/url.Userinfo nil,
		Host: "",
		Path: "/hello/42",
		RawPath: "",
		OmitHost: false,
		ForceQuery: false,
		RawQuery: "query=true",
		Fragment: "",
		RawFragment: "",},
	Proto: "HTTP/1.1",
	ProtoMajor: 1,
	ProtoMinor: 1,
	Header: net/http.Header [
		"User-Agent": [
			"Go-http-client/1.1",
		],
		"Authorization": [
			"Basic dXNlcjo=",
		],
		"Accept-Encoding": ["gzip"],
	],
	Body: io.ReadCloser(net/http.noBody) {},
	GetBody: nil,
	ContentLength: 0,
	TransferEncoding: []string len: 0, cap: 0, nil,
	Close: false,
	Host: "localhost:8080",
	Form: net/url.Values nil,
	PostForm: net/url.Values nil,
	MultipartForm: *mime/multipart.Form nil,
	Trailer: net/http.Header nil,
	RemoteAddr: "[::1]:59968",
	RequestURI: "/hello/42?query=true",
	TLS: *crypto/tls.ConnectionState nil,
	Cancel: <-chan struct {} {},
	Response: *net/http.Response nil,
	Pattern: "/hello/{id}",
	ctx: context.Context(*context.cancelCtx) *{
		Context: context.Context(*context.cancelCtx) ...,
		mu: (*sync.Mutex)(0x1400019c2e0),
		done: (*"sync/atomic.Value")(0x1400019c2e8),
		children: map[context.canceler]struct {} nil,
		err: error nil,
		cause: error nil,},
	pat: *net/http.pattern {                  <--------------------------- /** `pat` is filled **/
		str: "/hello/{id}",
		method: "",
		host: "",
		segments: []net/http.segment len: 2, cap: 2, [
			(*"net/http.segment")(0x1400007ec60),
			(*"net/http.segment")(0x1400007ec78),
		],
		loc: "/Users/manjong/Documents/sources/opentelemetry-go-instrumentatio...+38 more",},
	matches: []string len: 1, cap: 1, ["42"],
	otherValues: map[string]string nil,}

And this is the one for our Gin example. (pat is nil)

> net/http.(*conn).serve() /opt/homebrew/Cellar/go/1.23.1/libexec/src/net/http/server.go:2093 (PC: 0x10477abac)
Warning: debugging optimized function
Values returned:

  2088:			// in parallel even if their responses need to be serialized.
  2089:			// But we're not going to implement HTTP pipelining because it
  2090:			// was never deployed in the wild and the answer is HTTP/2.
  2091:			inFlightResponse = w
  2092:			serverHandler{c.server}.ServeHTTP(w, w.req)
=>2093:			inFlightResponse = nil
  2094:			w.cancelCtx()
  2095:			if c.hijacked() {
  2096:				return
  2097:			}
  2098:			w.finishRequest()
(dlv) p req
("*net/http.Request")(0x14000489180)
*net/http.Request {
	Method: "GET",
	URL: *net/url.URL {
		Scheme: "",
		Opaque: "",
		User: *net/url.Userinfo nil,
		Host: "",
		Path: "/hello-gin/1",
		RawPath: "",
		OmitHost: false,
		ForceQuery: false,
		RawQuery: "",
		Fragment: "",
		RawFragment: "",},
	Proto: "HTTP/1.1",
	ProtoMajor: 1,
	ProtoMinor: 1,
	Header: net/http.Header [
		"User-Agent": [
			"Go-http-client/1.1",
		],
		"Accept-Encoding": ["gzip"],
	],
	Body: io.ReadCloser(net/http.noBody) {},
	GetBody: nil,
	ContentLength: 0,
	TransferEncoding: []string len: 0, cap: 0, nil,
	Close: false,
	Host: "localhost:8080",
	Form: net/url.Values nil,
	PostForm: net/url.Values nil,
	MultipartForm: *mime/multipart.Form nil,
	Trailer: net/http.Header nil,
	RemoteAddr: "[::1]:60307",
	RequestURI: "/hello-gin/1",
	TLS: *crypto/tls.ConnectionState nil,
	Cancel: <-chan struct {} {},
	Response: *net/http.Response nil,
	Pattern: "",
	ctx: context.Context(*context.cancelCtx) *{
		Context: context.Context(*context.cancelCtx) ...,
		mu: (*sync.Mutex)(0x1400019a010),
		done: (*"sync/atomic.Value")(0x1400019a018),
		children: map[context.canceler]struct {} nil,
		err: error nil,
		cause: error nil,},
	pat: *net/http.pattern nil,            <--------------------------- /** `pat` is nil **/
	matches: []string len: 0, cap: 0, nil,
	otherValues: map[string]string nil,}

@RonFed
Copy link
Contributor

RonFed commented Dec 2, 2024

@iBluemind Thank you for the contribution!

I didn't go in-depth to the code yet, just addressing @MrAlias points:

  • I think the original reason we removed the gin probes is that we didn't collect any gin-specific data and those spans didn't add any extra data. This PR adds the http.route attribute for those frameworks which is something super useful and I saw a few users who would greatly benefit from these changes. So in general, if we can add attributes specific to gin or chi (which goes together with the semconv) - I think this has great value.
  • large PR - I agree - but a lot of it is offsets and test JSON data.
  • Custom Probes #1105 is a good point, however, as I said I do see a lot of value in adding more attributes to gin users.

@MrAlias
Copy link
Contributor

MrAlias commented Dec 2, 2024

I understand the name is different. But I'm not seeing that as justification in adding redundant telemetry as identified (and fixed by) #780.

@iBluemind
Copy link
Author

I agree that having 3 probes(nethttp, gin, chi) seems like a bit of redundancy. Is there a better way to do this?

@MrAlias
Copy link
Contributor

MrAlias commented Dec 2, 2024

I agree that having 3 probes(nethttp, gin, chi) seems like a bit of redundancy. Is there a better way to do this?

Can we update the nethttp instrumentation to detect what name parsing should be performed and signal the eBPF code with that information? I.e. detect if gin, chi, or net/http routing patterns should be used and send an enum as a flag.

@iBluemind
Copy link
Author

Can we update the nethttp instrumentation to detect what name parsing should be performed and signal the eBPF code with that information? I.e. detect if gin, chi, or net/http routing patterns should be used and send an enum as a flag.

That would be better, thank you

@iBluemind iBluemind closed this Dec 2, 2024
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.

3 participants