[build] cmd/resultdbpprof: add tool to get a test execution profile
Michael Knyszek (Gerrit)
Michael Knyszek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 1 comment![]( "Open in Gerrit")
go 1.24.9I'm not sure if this is OK. if it isn't, I can find an older version of pprof to depend on.
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted and added 6 comments![]( "Open in Gerrit")
Votes added by Dmitri Shuralyov
| Code-Review | +2 |
6 comments
package mainConsider adding a package comment for the command. Brief is fine. (Compare with https://golang.org/x/build/cmd.)
Edit: You've already resolved this in PS 2. :)
(nit) In x/build it's generally fine to have only 2 sections of imports: std lib and everything else. It is also fine to have it if this extra separation helps (or if it's simply not worth the extra iteration to drop).
flag.Usage()It's generally a nicer experience for users if we put usage information in flag.Usage too, so someone running the command with -help first would see it. See [an example](https://cs.opensource.google/go/x/build/+/master:cmd/genbootstrap/genbootstrap.go;l=52-63;drc=f142e550d08c506a75fdf65bb032731f0234675e) of what I mean.
buildID, err := strconv.ParseInt(flag.Arg(0), 10, 64)Looks like a missed err != nil check here.
I'm not sure if this is OK. if it isn't, I can find an older version of pprof to depend on.
We should keep it at 1.24 major version and not something higher (like 1.25) until 1.26.0 is out and 1.24 stops being supported by our policy. From that perspective, 1.24.9 is okay (i.e., it doesn't make it impossible to still use 1.24 for anyone still on it). Also, it seems the pprof dependency needed to require at least 1.24.9 because of a valid problem in 1.24.8 and older (https://github.com/google/pprof/pull/975), so this isn't a case of it being accidentally bumped without much of a reason.
So this should be okay. (It will propagate to x/website during a future auto-updating+tagging, which is expected and also okay.)
Related details
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 1 comment![]( "Open in Gerrit")
flag.Usage()It's generally a nicer experience for users if we put usage information in flag.Usage too, so someone running the command with -help first would see it. See [an example](https://cs.opensource.google/go/x/build/+/master:cmd/genbootstrap/genbootstrap.go;l=52-63;drc=f142e550d08c506a75fdf65bb032731f0234675e) of what I mean.
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michael Knyszek
| Auto-Submit | +1 |
| Commit-Queue | +1 |
(nit) In x/build it's generally fine to have only 2 sections of imports: std lib and everything else. It is also fine to have it if this extra separation helps (or if it's simply not worth the extra iteration to drop).
Done
flag.Usage()Michael KnyszekIt's generally a nicer experience for users if we put usage information in flag.Usage too, so someone running the command with -help first would see it. See [an example](https://cs.opensource.google/go/x/build/+/master:cmd/genbootstrap/genbootstrap.go;l=52-63;drc=f142e550d08c506a75fdf65bb032731f0234675e) of what I mean.
oops, yes, totally agree. I knew I forgot something...
Done
buildID, err := strconv.ParseInt(flag.Arg(0), 10, 64)Looks like a missed err != nil check here.
Done
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. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: cmd/resultdbpprof/main.go
Insertions: 26, Deletions: 4.
@@ -42,14 +42,13 @@
"time"
"github.com/google/pprof/profile"
- "golang.org/x/sync/errgroup"
- "google.golang.org/protobuf/types/known/fieldmaskpb"
-
"go.chromium.org/luci/auth"
bbpb "go.chromium.org/luci/buildbucket/proto"
"go.chromium.org/luci/grpc/prpc"
"go.chromium.org/luci/hardcoded/chromeinfra"
rdbpb "go.chromium.org/luci/resultdb/proto/v1"
+ "golang.org/x/sync/errgroup"
+ "google.golang.org/protobuf/types/known/fieldmaskpb"
)
var (
@@ -57,6 +56,24 @@
public = flag.Bool("public", true, "whether the build is public or not")
)
+func init() {
+ flag.Usage = func() {
+ fmt.Fprintf(flag.CommandLine.Output(), "Usage: %s [flags] <build ID>\n", os.Args[0])
+ fmt.Fprintf(flag.CommandLine.Output(), "\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "Downloads test results for a LUCI build and generates a pprof\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "profile of their execution times. Useful for understanding test\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "execution times and identifying low hanging fruit to speed up CI.\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "Results are written to '<build ID>.prof' in the current working\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "directory.\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "This tool expects Go test names of the form 'pkg.TestX/Y/Z'.\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "Test names not matching this pattern may appear in the output in\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "an unexpected form.\n")
+ fmt.Fprintf(flag.CommandLine.Output(), "\n")
+ flag.PrintDefaults()
+ }
+}
+
func main() {
// Validate flags.
flag.Parse()
@@ -67,7 +84,12 @@
flag.Usage()
os.Exit(1)
}
- buildID, err := strconv.ParseInt(flag.Arg(0), 10, 64)
+ idArg := flag.Arg(0)
+ idArg, _ = strings.CutPrefix(idArg, "b") // Allow optional 'b' prefix for easier copy-pasting.
+ buildID, err := strconv.ParseInt(idArg, 10, 64)
+ if err != nil {
+ log.Fatalf("parsing build ID %s: %v", flag.Arg(0), err)
+ }
// Create client.
ctx := context.Background()
```
Change information
cmd/resultdbpprof: add tool to get a test execution profile
- A cmd/resultdbpprof/main.go
- M go.mod
- M go.sum
Code-Review: +1 by Dmitri Shuralyov, +2 by Dmitri Shuralyov
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |