Show pprof labels in thread names #3501

stefanhaller · 2023-09-16T13:46:33Z

If a goroutine has labels, we add them after the ID.

aarzilli · 2023-09-16T19:49:53Z

service/dap/server.go

+				}
+				sort.Strings(keys)
+				for _, k := range keys {
+					labels += fmt.Sprintf(" %s=%s", k, g.Labels()[k])


use a strings.Builder

Ah yes, thanks. See 93d379b.

aarzilli

cc @hyangah @suzmue

stefanhaller · 2023-09-17T15:03:53Z

After testing this for a while I found that k1:v1 looks better than k1=v1, so I pushed a fixup (515f4cd) to change that. Not a big thing, let me know if you disagree.

derekparker · 2023-10-04T18:49:05Z

@hyangah or @suzmue would you mind taking a look at this?

stefanhaller · 2023-10-04T19:48:09Z

@derekparker Please note that over in #3493 some concerns were brought up; showing all labels might be too much when there are many. Please see the discussion there, starting at #3493 (comment).

suzmue · 2023-10-04T23:01:56Z

I think this looks great but would love to see this behind an opt-in configuration to start! (commented on #3493 as well)

alexandear · 2023-10-05T15:48:39Z

service/dap/server.go

+			var labels strings.Builder
+			if len(g.Labels()) > 0 {
+				var keys []string
+				for k := range g.Labels() {
+					keys = append(keys, k)
+				}
+				sort.Strings(keys)
+				for _, k := range keys {
+					fmt.Fprintf(&labels, " %s:%s", k, g.Labels()[k])
+				}
+			}


Can we rewrite it as follows:

Suggested change

var labels strings.Builder

if len(g.Labels()) > 0 {

var keys []string

for k := range g.Labels() {

keys = append(keys, k)

}

sort.Strings(keys)

for _, k := range keys {

fmt.Fprintf(&labels, " %s:%s", k, g.Labels()[k])

}

}

keys := make([]string, 0, len(g.Labels()))

for k := range g.Labels() {

keys = append(keys, k)

}

sort.Strings(keys)

var labels strings.Builder

for _, k := range keys {

labels.WriteByte(' ')

labels.WriteString(k)

labels.WriteByte(':')

labels.WriteString(g.Labels()[k])

}

?

The implementation has changed after the discussion in #3493, as we don't show all labels any more.

I'm curious though, why is writing individual bytes manually better than Fprintf to a builder?

stefanhaller · 2023-10-07T09:34:49Z

@suzmue I force-pushed to implement the config, please have a look!

suzmue · 2023-10-13T20:17:56Z

I think having a more general solution here could be better. I think having the flexibility to look at all the pprof labels would be great.

We could combine that with another config to only show specific labels but I don't think that needs to go into this change, but I either a list of labels to show or a string that can be parsed like goroutineFilters would be good. It would be nice to be able to show both a single label like the name use case, and a combination of labels that are interesting to the user.

We can also make the pprof labels config easier to adjust / more discoverable by adding it to the callstack context menu in VS Code. Once we get this change in here I am happy to add it (see hide system goroutines example)

stefanhaller · 2023-10-14T09:52:40Z

@suzmue Now I'm confused. 😅 Could you please be more specific as to what you would like me to do in this PR?

It sounds like you are thinking of having two different configs, where the second could be added later and would be some sort of filter for the labels. But then what's the first one? A bool option to enable the feature? But once we add the filter, would we still need the bool option?

I'm happy to implement the second option now, whatever it is, but I'd like to have agreement over the exact design before I start, in order to avoid wasting time. My proposal would be to add a single config, showPprofLabels, which is a list of strings, default empty. No globbing or regex, just a simple list of keys, because that simplifies things a lot. As a special case for my use case, I'd show just the value of the label if the length of the array is one, and k:v otherwise. The downside of this approach is that there's no easy way to say "show all keys" without listing them all manually; but we could add the special case of a single element of "*" for that, if needed.

Thoughts?

suzmue · 2023-10-18T21:17:07Z

@stefanhaller I think your idea of showPprofLabels as a list of strings sounds great! Those two special cases also sound good to me. Thanks!

stefanhaller · 2023-11-09T18:03:35Z

@suzmue I force pushed to implement the proposed solution.

stefanhaller · 2023-11-18T12:39:02Z

Ping. @suzmue, would you have time to have a look at this? (Or anybody else?)

suzmue · 2023-11-20T19:09:16Z

service/dap/server.go

@@ -1812,10 +1822,41 @@ func (s *Session) onThreadsRequest(request *dap.ThreadsRequest) {
 			if g.Thread != nil && g.Thread.ThreadID() != 0 {
 				thread = fmt.Sprintf(" (Thread %d)", g.Thread.ThreadID())
 			}
+			var labels strings.Builder
+			appendLabelsForKeys := func(keys []string) {


Maybe call this writeLabelsForKeys to be more clear?

Renamed in 49a0898

suzmue · 2023-11-20T19:39:02Z

service/dap/server_test.go

+						if len(tr.Body.Threads) != 1 {
+							t.Errorf("got %d threads, expected 1\n", len(tr.Body.Threads))
+						}
+						// The first breakpoint is before the call to pprof.Do; no lables yet:


typo: labels

Fixed in fb5bcaf

suzmue · 2023-11-20T19:41:37Z

service/dap/server_test.go

+		showPprofLabelsConfig   []string
+		expectedPrefixWithLabel string
+	}{
+		{


Since these test cases aren't super long, could we define them on a single line instead? eg:

{[]string{}, "* [Go 1]"}, {[]string{"k1"}, "* [Go 1 v1]"}, ...

Sure, reformatted in d5ac1c6

suzmue · 2023-11-20T20:01:47Z

service/dap/config.go

@@ -85,3 +94,37 @@ func configureSetSubstitutePath(args *launchAttachArgs, rest string) error {
 	}
 	return nil
 }
+
+func configureSetShowPprofLabels(args *launchAttachArgs, rest string) error {


Could you add documentation to command.go explaining how this configuration works please? For msgConfig

Added in 285116f

stefanhaller · 2023-11-23T19:33:38Z

Thanks for the review, I addressed all requests. I referenced the individual fixup commits above to make it easier to see what I changed, but I'm going to squash them now (and rebase on master again).

The config is a string value that indicates the key of a pprof label whose value should be shown as a goroutine name in the threads view.

stefanhaller · 2023-11-30T17:54:07Z

I force-pushed again to fix the conflicts with master.

stefanhaller mentioned this pull request Sep 16, 2023

dap: show pprof labels in thread names #3493

Closed

aarzilli reviewed Sep 16, 2023

View reviewed changes

aarzilli approved these changes Sep 17, 2023

View reviewed changes

derekparker added the awaiting-dap-ack label Sep 19, 2023

alexandear reviewed Oct 5, 2023

View reviewed changes

stefanhaller force-pushed the show-labels-for-goroutines branch from 515f4cd to a199d7b Compare October 7, 2023 09:32

stefanhaller force-pushed the show-labels-for-goroutines branch from a199d7b to ca4204b Compare November 9, 2023 17:48

suzmue suggested changes Nov 20, 2023

View reviewed changes

stefanhaller force-pushed the show-labels-for-goroutines branch from 285116f to e5d5e6e Compare November 23, 2023 19:42

stefanhaller added 2 commits November 30, 2023 18:47

Cleanup: run gofumpt on server.go and server_test.go

eec3e48

Add pprofLabelForThreadNames config

6715605

The config is a string value that indicates the key of a pprof label whose value should be shown as a goroutine name in the threads view.

stefanhaller force-pushed the show-labels-for-goroutines branch from e5d5e6e to 6715605 Compare November 30, 2023 17:51

stefanhaller requested a review from suzmue November 30, 2023 17:54

hyangah approved these changes Dec 4, 2023

View reviewed changes

aarzilli merged commit f8c8b33 into go-delve:master Dec 4, 2023
1 of 2 checks passed

stefanhaller deleted the show-labels-for-goroutines branch December 9, 2023 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show pprof labels in thread names #3501

Show pprof labels in thread names #3501

stefanhaller commented Sep 16, 2023

aarzilli Sep 16, 2023

stefanhaller Sep 17, 2023

aarzilli left a comment

stefanhaller commented Sep 17, 2023

derekparker commented Oct 4, 2023

stefanhaller commented Oct 4, 2023

suzmue commented Oct 4, 2023

alexandear Oct 5, 2023

stefanhaller Oct 7, 2023

stefanhaller commented Oct 7, 2023

suzmue commented Oct 13, 2023

stefanhaller commented Oct 14, 2023

suzmue commented Oct 18, 2023

stefanhaller commented Nov 9, 2023

stefanhaller commented Nov 18, 2023

suzmue Nov 20, 2023

stefanhaller Nov 23, 2023

suzmue Nov 20, 2023

stefanhaller Nov 23, 2023

suzmue Nov 20, 2023

stefanhaller Nov 23, 2023

suzmue Nov 20, 2023

stefanhaller Nov 23, 2023

stefanhaller commented Nov 23, 2023

stefanhaller commented Nov 30, 2023

Show pprof labels in thread names #3501

Show pprof labels in thread names #3501

Conversation

stefanhaller commented Sep 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment

Choose a reason for hiding this comment

stefanhaller commented Sep 17, 2023

derekparker commented Oct 4, 2023

stefanhaller commented Oct 4, 2023

suzmue commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhaller commented Oct 7, 2023

suzmue commented Oct 13, 2023

stefanhaller commented Oct 14, 2023

suzmue commented Oct 18, 2023

stefanhaller commented Nov 9, 2023

stefanhaller commented Nov 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhaller commented Nov 23, 2023

stefanhaller commented Nov 30, 2023