extract multiple metrics from golang benchmarks #119
Conversation
allows go benchmarks to work with extra metrics
|
@ktrz Hey are we going to proceed this PR? This change is important to unblock our use case. |
|
I'll say from my perspective i'm actually getting ready to use this action for a second time, so i'm available and happy to iterate on any feedack, @ktrz. |
|
Maybe it is useful to clarify the scope in PR description. For example, only output the raw data but not working on diagram now? |
There was a problem hiding this comment.
Thank you @joe-kimmel-vmw for the PR! I've added a couple of comments. Also, I have a question on how the auxiliary metrics will/should affect if the given run passes or fails. Since the units are not strictly defined it's hard to know immediately if bigger number is better or smaller is better.
| - uses: actions/setup-go@v1 | ||
| with: | ||
| go-version: "1.17.6" |
There was a problem hiding this comment.
Do we depend on a specific version of Go for this feature to work? Maybe it's worth upgrading the actions/setup-go to v4 and using go-version: 'stable'?
| - uses: actions/setup-go@v1 | |
| with: | |
| go-version: "1.17.6" | |
| - uses: actions/setup-go@v4 | |
| with: | |
| go-version: "stable" |
| for i := 0; i < b.N; i++ { | ||
| var _ = Fib(10) | ||
| } | ||
| b.ReportMetric(3.0, "auxMetricUnits") | ||
| } | ||
|
|
||
| func BenchmarkFib20(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| var _ = Fib(20) | ||
| } | ||
| b.ReportMetric(4.0, "auxMetricUnits") | ||
| } |
There was a problem hiding this comment.
I would actually suggest creating a third benchmark function that reports additional metrics instead of modifying the existing ones
| for i := 0; i < b.N; i++ { | |
| var _ = Fib(10) | |
| } | |
| b.ReportMetric(3.0, "auxMetricUnits") | |
| } | |
| func BenchmarkFib20(b *testing.B) { | |
| for i := 0; i < b.N; i++ { | |
| var _ = Fib(20) | |
| } | |
| b.ReportMetric(4.0, "auxMetricUnits") | |
| } | |
| for i := 0; i < b.N; i++ { | |
| var _ = Fib(10) | |
| } | |
| } | |
| func BenchmarkFib20(b *testing.B) { | |
| for i := 0; i < b.N; i++ { | |
| var _ = Fib(20) | |
| } | |
| } | |
| func BenchmarkFib20WithAuxMetric(b *testing.B) { | |
| for i := 0; i < b.N; i++ { | |
| var _ = Fib(20) | |
| } | |
| b.ReportMetric(4.0, "auxMetricUnits") | |
| } |
| // reference, "Proposal: Go Benchmark Data Format": https://go.googlesource.com/proposal/+/master/design/14313-benchmark-format.md | ||
| // "A benchmark result line has the general form: <name> <iterations> <value> <unit> [<value> <unit>...]" | ||
| // "The fields are separated by runs of space characters (as defined by unicode.IsSpace), so the line can be parsed with strings.Fields. The line must have an even number of fields, and at least four." | ||
| const reExtract = /^(Benchmark\w+(?:\/?[\w()$%^&*-]*?)*?)(-\d+)?\s+(\d+)\s+(.+)$/; |
There was a problem hiding this comment.
You've changed this regexp quite significantly. Could you explain a bit why is this so? I thought the metrics might appear at the end of the currently handled cases so this shorter regexp is a little surprising. If you've found a way to simplify it then great but a little explanation would go a long way :)
Additionally, could we use named capture groups (link so it's easier to understand the parts of the regular expression
|
I've merged couple of smaller PRs. Could you please resolve the conflicts? And also this feature might affect your regexp (it should be covered with tests though) |
|
@joe-kimmel-vmw Are you still interested in revising this PR? I could help you review and merge it if you can address the comments. |
|
Close as resolved. |
Hello and thanks for this repo / tool - much appreciated!
This is one possible approach to address the issue I filed :
allows go benchmarks to work with extra metrics
Happy to iterate or change approaches per guidance if there's interest.
thanks!