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

Remove gin specific instrumentation #738

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Apr 12, 2024

The gin specific instrumentation shouldn't be needed anymore. We set our probes on net/http.serverHandler.ServeHTTP and it's what calls the Gin method we were instrumenting.

Here's the output from Delve, stopped at our Gin instrumentation point:

569:	// ServeHTTP conforms to the http.Handler interface.
=> 570:	func (engine *Engine) ServeHTTP(w http.ResponseWriter, req *http.Request) {
   571:		c := engine.pool.Get().(*Context)
   572:		c.writermem.reset(w)
   573:		c.Request = req
   574:		c.reset()
   575:	
(dlv) bt
0  0x0000000000931f4e in github.com/gin-gonic/gin.(*Engine).ServeHTTP
   at ./vendor/github.com/gin-gonic/gin/gin.go:570
1  0x000000000071844e in net/http.serverHandler.ServeHTTP < ** we instrument this already **
   at /home/nino/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.1.linux-amd64/src/net/http/server.go:3137
2  0x0000000000713728 in net/http.(*conn).serve
   at /home/nino/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.1.linux-amd64/src/net/http/server.go:2039
3  0x0000000000718c68 in net/http.(*Server).Serve.gowrap3
   at /home/nino/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.1.linux-amd64/src/net/http/server.go:3285
4  0x0000000000472bc1 in runtime.goexit
   at /home/nino/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.1.linux-amd64/src/runtime/asm_amd64.s:1695

@grcevski grcevski merged commit 9588a7c into grafana:main Apr 13, 2024
4 checks passed
@grcevski grcevski deleted the remove_gin_instr branch April 13, 2024 12:38
mattdurham pushed a commit to mattdurham/beyla that referenced this pull request Jan 22, 2025
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.

2 participants