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

[feature] Instrument goProbe with metrics #174

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

els0r
Copy link
Owner

@els0r els0r commented Aug 7, 2023

Closes #173

@els0r els0r requested a review from fako1024 August 7, 2023 20:46
@els0r els0r mentioned this pull request Aug 8, 2023
1 task
Copy link
Collaborator

@fako1024 fako1024 left a comment

Choose a reason for hiding this comment

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

Would appreciate if you could take a look at the feedback. Aside from that I've observed that when enabling profiling and metrics in the config:

   "api": {
     "addr": "127.0.0.1:3005",
     "metrics": true,
     "profiling": true,
   },

and I then try to pull a CPU profile (no matter if via the browser or via go tool pprof) I always get an empty profile (no error, just no samples; heap etc. work just fine). Can you confirm this? Am I missing something here?

pkg/api/globalquery/server/server.go Outdated Show resolved Hide resolved
pkg/api/goprobe/server/server.go Outdated Show resolved Hide resolved
pkg/api/server/server.go Show resolved Hide resolved
pkg/capture/capture.go Show resolved Hide resolved
pkg/capture/metrics.go Outdated Show resolved Hide resolved
pkg/goprobe/writeout/metrics.go Outdated Show resolved Hide resolved
cmd/goProbe/goProbe.go Show resolved Hide resolved
@els0r
Copy link
Owner Author

els0r commented Aug 8, 2023

Would appreciate if you could take a look at the feedback. Aside from that I've observed that when enabling profiling and metrics in the config:

   "api": {
     "addr": "127.0.0.1:3005",
     "metrics": true,
     "profiling": true,
   },

and I then try to pull a CPU profile (no matter if via the browser or via go tool pprof) I always get an empty profile (no error, just no samples; heap etc. work just fine). Can you confirm this? Am I missing something here?

I'll double check with your configuration. But I specifically used a /pprof endpoint to test an API related metric, so not sure what's up in your case.

The path is /debug/pprof/profile for a CPU profile. Did you use that?

@fako1024
Copy link
Collaborator

fako1024 commented Aug 8, 2023

Would appreciate if you could take a look at the feedback. Aside from that I've observed that when enabling profiling and metrics in the config:

   "api": {
     "addr": "127.0.0.1:3005",
     "metrics": true,
     "profiling": true,
   },

and I then try to pull a CPU profile (no matter if via the browser or via go tool pprof) I always get an empty profile (no error, just no samples; heap etc. work just fine). Can you confirm this? Am I missing something here?

I'll double check with your configuration. But I specifically used a /pprof endpoint to test an API related metric, so not sure what's up in your case.

The path is /debug/pprof/profile for a CPU profile. Did you use that?

Jup, I used this command line call (on top of just navigating via the browser):

go tool pprof http://127.0.0.1:3005/debug/pprof/profile

And it definitively triggers the profile and pulls it (takes 30s by default, and I can set the time using the seconds query parameter), it's just always empty...

@els0r
Copy link
Owner Author

els0r commented Aug 8, 2023

Jup, I used this command line call (on top of just navigating via the browser):

go tool pprof http://127.0.0.1:3005/debug/pprof/profile

And it definitively triggers the profile and pulls it (takes 30s by default, and I can set the time using the seconds query parameter), it's just always empty...

Could you please run

curl  http://127.0.0.1:3005/debug/pprof/profile\?seconds=300 -o cpu.profile

and see if it still occurs? I was also getting empty profiles for 30 seconds (using curl), while the profile for 300s was there, but sparse. I ran it on a node with little traffic, so this may just be a case of insufficient sampling.

@els0r
Copy link
Owner Author

els0r commented Aug 8, 2023

@fako1024 : feedback amended. Ready when you are.

Copy link
Collaborator

@fako1024 fako1024 left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback, LGTM!

@fako1024
Copy link
Collaborator

fako1024 commented Aug 9, 2023

Jup, I used this command line call (on top of just navigating via the browser):

go tool pprof http://127.0.0.1:3005/debug/pprof/profile

And it definitively triggers the profile and pulls it (takes 30s by default, and I can set the time using the seconds query parameter), it's just always empty...

Could you please run

curl  http://127.0.0.1:3005/debug/pprof/profile\?seconds=300 -o cpu.profile

and see if it still occurs? I was also getting empty profiles for 30 seconds (using curl), while the profile for 300s was there, but sparse. I ran it on a node with little traffic, so this may just be a case of insufficient sampling.

Interesting - indeed after 300s I get something:

Duration: 300.01s, Total samples = 140ms (0.047%)

However, that's quite an aggressive sampling rate. There's also this issue gin-contrib/pprof#10 (which was closed with a hint at sampling). Given the fact that a "normal" profile run with StartCPUProfile() yields a much better sampling rate I think we need to add a call to runtime.SetCPUProfileRate(hz) (because that's what is being done in StartCPUProfile()): https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/runtime/pprof/pprof.go;l=771

Belay that, the pprof wrapper calls StartCPUProfile(), so apparently I was just being dumb here. All good.

@els0r els0r merged commit c5f76fc into develop Aug 9, 2023
5 checks passed
@els0r els0r deleted the feature/instrument-goprobe-with-metrics branch August 9, 2023 16:58
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.

Instrument goProbe with metrics
2 participants