[tools] internal/server: add instrumentation to track vulncheck codelens usage
Ethan Lee (Gerrit)
Ethan Lee has uploaded the change for review![]( "Open in Gerrit")
Commit message
internal/server: add instrumentation to track vulncheck codelens usage
- Instrument users' clicking on vulncheck codelens.
Change diff
diff --git a/gopls/internal/mod/code_lens.go b/gopls/internal/mod/code_lens.go
index 038fd20..2373830 100644
--- a/gopls/internal/mod/code_lens.go
+++ b/gopls/internal/mod/code_lens.go
@@ -164,8 +164,9 @@
}
vulncheck := command.NewVulncheckCommand("Run govulncheck", command.VulncheckArgs{
- URI: uri,
- Pattern: "./...",
+ URI: uri,
+ Pattern: "./...",
+ FromCodeLens: true,
})
return []protocol.CodeLens{
{Range: rng, Command: vulncheck},
@@ -192,8 +193,9 @@
}
vulncheck := command.NewRunGovulncheckCommand("Run govulncheck", command.VulncheckArgs{
- URI: uri,
- Pattern: "./...",
+ URI: uri,
+ Pattern: "./...",
+ FromCodeLens: true,
})
return []protocol.CodeLens{
{Range: rng, Command: vulncheck},
diff --git a/gopls/internal/protocol/command/interface.go b/gopls/internal/protocol/command/interface.go
index f3e6f01..d34a2f8 100644
--- a/gopls/internal/protocol/command/interface.go
+++ b/gopls/internal/protocol/command/interface.go
@@ -528,6 +528,9 @@
// Package pattern. E.g. "", ".", "./...".
Pattern string
+ // Record if vulncheck has been been triggered via codelens.
+ FromCodeLens bool
+
// TODO: -tests
}
diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go
index 532a353..9407d71 100644
--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -1236,6 +1236,10 @@
dir := args.URI.DirPath()
pattern := args.Pattern
+ if args.FromCodeLens {
+ countVulncheckLensesMethod.Inc()
+ }
+
result, err := scan.RunGovulncheck(ctx, pattern, deps.snapshot, dir, workDoneWriter)
if err != nil {
return err
@@ -1322,6 +1326,10 @@
dir := filepath.Dir(args.URI.Path())
pattern := args.Pattern
+ if args.FromCodeLens {
+ countRunGovulncheckLensesMethod.Inc()
+ }
+
result, err := scan.RunGovulncheck(ctx, pattern, deps.snapshot, dir, workDoneWriter)
if err != nil {
return err
diff --git a/gopls/internal/server/counters.go b/gopls/internal/server/counters.go
index 72095d7..388eb92 100644
--- a/gopls/internal/server/counters.go
+++ b/gopls/internal/server/counters.go
@@ -43,3 +43,10 @@
countRemoveStructTags = counter.New("gopls/structtags:remove")
)
+
+// Proposed counters for mod codelens usage. These counters
+// increment when the user attempts invokes a go.mod codelens.
+var (
+ countVulncheckLensesMethod = counter.New("gopls/mod/codelenses:vulncheck")
+ countRunGovulncheckLensesMethod = counter.New("gopls/mod/codelenses:go_vulncheck")
+)
Change information
- M gopls/internal/mod/code_lens.go
- M gopls/internal/protocol/command/interface.go
- M gopls/internal/server/command.go
- M gopls/internal/server/counters.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
FromCodeLens: true,since the codelens counter is for all the go mod file codelenses, maybe we can put a more generalized field here, like "source".
maybe we need to re-think about the model, maybe we should do something like
```
gopls/executeCommand/source:{vulncheck-codelens,go_vulncheck-codelens}
```
so we can extend this to `vulncheck-codeaction` and other command that can be triggered from both code action, codelens or even vscode command pallette.
I'm very sorry that during our previous conversation, I did not think deeper enough into how the gopls command execution works.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 1 comment![]( "Open in Gerrit")
FromCodeLens: true,since the codelens counter is for all the go mod file codelenses, maybe we can put a more generalized field here, like "source".
maybe we need to re-think about the model, maybe we should do something like
```
gopls/executeCommand/source:{vulncheck-codelens,go_vulncheck-codelens}
```so we can extend this to `vulncheck-codeaction` and other command that can be triggered from both code action, codelens or even vscode command pallette.
I'm very sorry that during our previous conversation, I did not think deeper enough into how the gopls command execution works.
I see what you mean and it is a good idea to generalize this so that we can capture both the command and its trigger.
Let's discuss this further and devise a more scalable method.
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 1 comment![]( "Open in Gerrit")
Ethan Leesince the codelens counter is for all the go mod file codelenses, maybe we can put a more generalized field here, like "source".
maybe we need to re-think about the model, maybe we should do something like
```
gopls/executeCommand/source:{vulncheck-codelens,go_vulncheck-codelens}
```so we can extend this to `vulncheck-codeaction` and other command that can be triggered from both code action, codelens or even vscode command pallette.
I'm very sorry that during our previous conversation, I did not think deeper enough into how the gopls command execution works.
I see what you mean and it is a good idea to generalize this so that we can capture both the command and its trigger.
Let's discuss this further and devise a more scalable method.
Discussed offline regarding generalizing instrumentation for different commands and sources.
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 2 comments![]( "Open in Gerrit")
Source: command.CodeLens,I think we should do it in a different place instead of here. Let's make it more general.
`server/code_lens.go` is a better place which will handle not only the go mod file but also other go files...
But maybe the best place is `gopls/internal/protocol/tsserver.go`
```
case "textDocument/codeLens":
var params CodeLensParams
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
resp, err := server.CodeLens(ctx, ¶ms)
if err != nil {
return nil, true, err
}
// TODO: let's example the response, if any of the codelen.command
// is not nil, let's add the "source: codelens" to it.
return resp, true, nil
```
Same can also be applied to codeaction. You can leave it a TODO there if you think this is beyond the scope.
But the tsserver.go is auto generate, please take a look at the auto-gen code to make the change.
if source := getCommandSource(params.Arguments); source != "" {
commandName := strings.TrimPrefix(params.Command, "gopls.")
counterName := fmt.Sprintf("gopls/cmd/source:%s-%s", commandName, source)
counter.New(counterName).Inc()
}Same thing here. I think this is a reasonable place. Since all the command execution will arrive in this place.
But if we are already making changes in tsserver.go, it make more sense to handle both "source" injection and "source" retrieval in there.
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 2 comments![]( "Open in Gerrit")
I think we should do it in a different place instead of here. Let's make it more general.
`server/code_lens.go` is a better place which will handle not only the go mod file but also other go files...
But maybe the best place is `gopls/internal/protocol/tsserver.go`
```
case "textDocument/codeLens":
var params CodeLensParams
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
resp, err := server.CodeLens(ctx, ¶ms)
if err != nil {
return nil, true, err
}
// TODO: let's example the response, if any of the codelen.command
// is not nil, let's add the "source: codelens" to it.
return resp, true, nil```
Same can also be applied to codeaction. You can leave it a TODO there if you think this is beyond the scope.
But the tsserver.go is auto generate, please take a look at the auto-gen code to make the change.
Sure, let's leave a TODO until we want to add instrumentation for codelens invocation.
if source := getCommandSource(params.Arguments); source != "" {
commandName := strings.TrimPrefix(params.Command, "gopls.")
counterName := fmt.Sprintf("gopls/cmd/source:%s-%s", commandName, source)
counter.New(counterName).Inc()
}Same thing here. I think this is a reasonable place. Since all the command execution will arrive in this place.
But if we are already making changes in tsserver.go, it make more sense to handle both "source" injection and "source" retrieval in there.
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- Ethan Lee
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted Commit-Queue+1![]( "Open in Gerrit")
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
You need rebase to fix the test issues in the x/tools module.
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ethan Lee
| Auto-Submit | +1 |
| Commit-Queue | +1 |
You need rebase to fix the test issues in the x/tools module.
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
args["source"] = "codelens"I had a chat with Alan today on this topic. I made a wrong suggestion to your CL and I'm very sorry for this confusion.
So, it's actually better to put it in the server/code_lens.go and server/command.go.
The generator should only contains the protocol level changes. We can have a function at the end of the codelens that inject a "source" to all the Command.
and we can have a similar function for workspace/executeCommand that retrieve the source from the Command.
Please also take a look and see if we can get rid of the source argument once we receive it. Let it serve as a middle ware so the command writer will not observer this argument ever.
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee added 1 comment![]( "Open in Gerrit")
I had a chat with Alan today on this topic. I made a wrong suggestion to your CL and I'm very sorry for this confusion.
So, it's actually better to put it in the server/code_lens.go and server/command.go.
The generator should only contains the protocol level changes. We can have a function at the end of the codelens that inject a "source" to all the Command.
and we can have a similar function for workspace/executeCommand that retrieve the source from the Command.
Please also take a look and see if we can get rid of the source argument once we receive it. Let it serve as a middle ware so the command writer will not observer this argument ever.
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- Ethan Lee
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
Related details
- Ethan Lee
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted![]( "Open in Gerrit")
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- Ethan Lee
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted![]( "Open in Gerrit")
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 2 comments![]( "Open in Gerrit")
if err := json.Unmarshal(lenses[i].Command.Arguments[0], &args); err == nil {If there is a command does not have any arguments, this code will not append the source to it right?
This make me think is this the right way of injecting? Should we try to pre-pend a new argument in front?
The argument list is a slice, so we can say, the last one is preserved for internal use only. We can create a type say `type metadata struct { source string }`.
Then we can try to unmarshall the last argument as this type, if it fails, no op.
If it succeed, take the last one out.
WDYT.
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ethan Lee
| Auto-Submit | +1 |
2 comments
if err := json.Unmarshal(lenses[i].Command.Arguments[0], &args); err == nil {If there is a command does not have any arguments, this code will not append the source to it right?
This make me think is this the right way of injecting? Should we try to pre-pend a new argument in front?
The argument list is a slice, so we can say, the last one is preserved for internal use only. We can create a type say `type metadata struct { source string }`.
Then we can try to unmarshall the last argument as this type, if it fails, no op.
If it succeed, take the last one out.
WDYT.
That's a good point. Yes, even if there are no arguments we can still append an extra argument for the source, which we will then use in executeCommand and finally remove from the argument slice.
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee uploaded new patchset![]( "Open in Gerrit")
- Ethan Lee
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ethan Lee (Gerrit)
Ethan Lee voted![]( "Open in Gerrit")
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Code-Review+2![]( "Open in Gerrit")
- Ethan Lee
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
internal/server: add instrumentation to track gopls command usage
- Add source field to args in server/code_lens
- Interpet and log source field in executeComand
- M gopls/internal/server/code_lens.go
- M gopls/internal/server/command.go
Code-Review: +2 by Hongxiang Jiang
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |