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

dap: show pprof labels in thread names #3493

Closed
stefanhaller opened this issue Sep 3, 2023 · 11 comments · Fixed by #3501
Closed

dap: show pprof labels in thread names #3493

stefanhaller opened this issue Sep 3, 2023 · 11 comments · Fixed by #3501

Comments

@stefanhaller
Copy link
Contributor

stefanhaller commented Sep 3, 2023

It would be useful to see pprof labels in VS Code's debugger. One possible way to show them is to append them after the go routine ID in the thread names, e.g. instead of showing just [Go 42], show [Go 42 key1: value1, key2: value2].

image
  1. What version of Delve are you using (dlv version)? 1.20.2
  2. What version of Go are you using? (go version)? go1.20.7 darwin/arm64
  3. What operating system and processor architecture are you using? MacOS 13.5.1, M1
@stefanhaller
Copy link
Contributor Author

Here's a PR: #3501

@hyangah
Copy link
Contributor

hyangah commented Sep 18, 2023

@stefanhaller Can you please include a screenshot for this?
The pprof label list can grow long and I wonder if this is the best way to present this info.

@stefanhaller
Copy link
Contributor Author

I added a screenshot to the PR description; it's the one from the goroutineLabels.go test fixture, not sure how useful it is.

I agree that it could be a problem if there are many labels. Do you have any suggestion how else to show them? I find it essential that they are visible directly in the stacktrace view so that it's easy to see which go routine is which (as opposed to having to hover over things to see the information, for example).

@suzmue
Copy link
Contributor

suzmue commented Sep 25, 2023

@stefanhaller I would love to understand more about your use case. When you are debugging, when would you most commonly want to refer to these labels? Are you using them to identify specific goroutines and navigate between them? Would moving the keys to after the function name make them less useful to you? If you can get a screenshot from a more realistic use case than the test fixture that would be very helpful as well.
As @hyangah mentioned, these lists could become really long. We use a lot of pprof labels internally and it seems likely these will take all the visible space in the stack frame.

For the implementation, it would also be great if we can make sure that loading the labels doesn't have a significant impact on the latency of the threads request.

@stefanhaller
Copy link
Contributor Author

OK, I see that I might be abusing labels for something that they weren't meant for.

My real goal is to identify goroutines by name in the call stack view. In C/C++ programs I can give names to threads using the pthread_setname_np function; these then appear in the debugger, which is useful to identify threads when they are all collapsed. Often, most threads block on a mutex or something similar, so they all look the same, and to find a specific thread I have to unfold them all to find the one I'm interested in. Thread names help a lot in this case.

Now I want the same thing for go, and since golang doesn't have a feature to set a name for a goroutine, I figured that pprof labels would be the closest approximation. The code base that I mostly work with doesn't use labels yet, so it would be easy to assign one key/value pair to each goroutine to identify it. Actually I don't care about the key, it would be always something like "name" for me, which is a bit pointless.

I do realize that this wouldn't work well for code bases that already make heavy use of labels. So maybe it needs to be an opt-in feature with a config? The config could be a list of labels to show in the debugger, or maybe even just a single label, where it would then only show the value and not the key. This latter option would work best for me, I guess.

@stefanhaller
Copy link
Contributor Author

Come to think of it, could it perhaps be acceptable to always show the value of the "name" label if there is one, without an opt-in config?

@suzmue
Copy link
Contributor

suzmue commented Oct 4, 2023

Ok sounds good, thanks for the info! Given the use case, I think the placement probably makes sense, since it will be more differentiating than the function name.

I think starting with an opt in config would be good and we can start trying out the UX with different projects that are using pprof labels more regularly.

@stefanhaller
Copy link
Contributor Author

Cool. Any suggestions for the name of the config? I can only think of awkwardly long ones like pprofLabelToShowAsThreadNames? (To be clear, I want to go with the option that uses a single key in the config and shows just its value in the thread view, as this is the most useful approach for my use case.)

@stefanhaller
Copy link
Contributor Author

For lack of better suggestions I went with pprofLabelForThreadNames for now. I force-pushed the branch in #3501 to implement this.

@hyangah
Copy link
Contributor

hyangah commented Oct 7, 2023

Sorry that I was late. I wonder if anyone considered

showPprofLabels which takes a list of keys or a regexp (e.g .* can be all keys found) to show in the thread view.

@stefanhaller
Copy link
Contributor Author

I wonder if anyone considered

showPprofLabels which takes a list of keys or a regexp (e.g .* can be all keys found) to show in the thread view.

Yes, I did consider this, and I mentioned it above. For me this is less ideal though, because in this case we'd have to show both the keys and the values, and in case you configure only one label that's unnecessary noise. For example, if you choose "name" as the key for all your thread names, then it would show [Go 1 name:myWorker], instead of just [Go 1 myWorker] which I would prefer.

That said, if everybody agrees that this more flexible solution is what we want, I could do that too. I have to admit that I'm a bit unsure how decision making works around here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants