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: Add tracing integration to profiling. #3276

Merged
merged 9 commits into from
Feb 6, 2024

Conversation

cyriltovena
Copy link
Contributor

What this PR does:

This adds tracing integration to pprof profiling. Every root span ID per service will be added to the profiling data using pprof labels and then forward to Pyroscope.

This allows to show flamegraph for a given trace

image

Which can be useful to understand resource usage per trace.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@cyriltovena
Copy link
Contributor Author

Not sure why windows build is failing.

 error=failed to build for windows_amd64_v1: exit status 1
....
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:22:6: getConf redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:6:6: other declaration of getConf
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:28:6: reloadConf redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:10:6: other declaration of reloadConf
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:133:6: overrideEnvWithStaticProxy redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:14:6: other declaration of overrideEnvWithStaticProxy
vendor/github.com/mattn/go-ieproxy/pac_windows.go:9:29: method ProxyScriptConf.findProxyForURL already declared at vendor/github.com/mattn/go-ieproxy/pac_unix.go:6:29

@knylander-grafana
Copy link
Contributor

@cyriltovena I saw that you checked the Documentation added box. Did you update doc or just add the changelog entry? If the latter, we can make a doc issue to update the Tempo doc and other doc sets, as needed.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Generally looks good but I think you need to run:

make update-mod

to tidy up the serverless go.mods

@cyriltovena
Copy link
Contributor Author

@cyriltovena I saw that you checked the Documentation added box. Did you update doc or just add the changelog entry? If the latter, we can make a doc issue to update the Tempo doc and other doc sets, as needed.

I don't think this needs to be mentioned in the doc. This is why I checked that box. I think a changelog entry is enough yes.

@mdisibio
Copy link
Contributor

Not sure why windows build is failing.

 error=failed to build for windows_amd64_v1: exit status 1
....
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:22:6: getConf redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:6:6: other declaration of getConf
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:28:6: reloadConf redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:10:6: other declaration of reloadConf
vendor/github.com/mattn/go-ieproxy/ieproxy_windows.go:133:6: overrideEnvWithStaticProxy redeclared in this block
	vendor/github.com/mattn/go-ieproxy/ieproxy_unix.go:14:6: other declaration of overrideEnvWithStaticProxy
vendor/github.com/mattn/go-ieproxy/pac_windows.go:9:29: method ProxyScriptConf.findProxyForURL already declared at vendor/github.com/mattn/go-ieproxy/pac_unix.go:6:29

We are getting this on other PRs too. This fix works locally, I will test it on my PR:

github.com/mattn/go-ieproxy v0.0.11 // indirect

chaudum pushed a commit to grafana/loki that referenced this pull request Jan 12, 2024
Same as grafana/tempo#3276 this adds profiling
integration to tracing instrumentation allowing to get profile for a
single request removing the noise of everything else.
@cyriltovena
Copy link
Contributor Author

@mdisibio should be good now thanks !

@cyriltovena
Copy link
Contributor Author

hey that's seems stale ?

@joe-elliott
Copy link
Member

@cyriltovena i'm good on this PR. if you can work out the conflicts we'll merge

@joe-elliott joe-elliott merged commit 3231863 into main Feb 6, 2024
15 checks passed
@joe-elliott joe-elliott deleted the feat/tracing-profiling-integration branch February 6, 2024 14:57
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
Same as grafana/tempo#3276 this adds profiling
integration to tracing instrumentation allowing to get profile for a
single request removing the noise of everything else.
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.

4 participants