-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
workload: extend the command-line interface #37929
Conversation
5f94bc2
to
c21cce9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you talk a little more about the motivation behind this? The --skip-final-report
makes me wonder if you're planning on parsing these output lines. If that's the case, I'd much prefer that we introduce a --json
option
Also add a release note please. workload run
is part of our public api now.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @knz)
pkg/workload/cli/run.go, line 61 at r1 (raw file):
var pprofport = initFlags.Int("pprofport", 33333, "Port for pprof endpoint.") var disp = runFlags.Duration("display-every", time.Second, "How much time between every one-line activity reports.")
nit: Avoid abbreviations please: s/disp/displayEvery/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you talk a little more about the motivation behind this?
I added more details in the (new) release note.
The --skip-final-report makes me wonder if you're planning on parsing these output lines. If that's the case, I'd much prefer that we introduce a --json option
The change applies both to the textual output and the JSON output, so I am not sure what you are asking?
Reviewable status: complete! 0 of 0 LGTMs obtained
pkg/workload/cli/run.go, line 61 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
nit: Avoid abbreviations please: s/disp/displayEvery/
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused about the tooling that you're building which needs these changes. Why does the textual output need to be absolute time? Why can't the tooling ignore the summary line? Flags add complexity to the API and, especially since this part of the public surface area of the cockroach cli, I'd like to be sure the complexity is warranted
Reviewable status: complete! 0 of 0 LGTMs obtained
My understanding is that you need this for your MVP for scenario-based testing, but I thought that MVP was not going to be checked in, which means that we also shouldn't check in aux changes required by it. I share Dan's concern that these flags are public complexity that I'm not sure is justified (at this moment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'll just leave this PR open until further clarity is delivered.
Reviewable status: complete! 0 of 0 LGTMs obtained
1f263a4
to
d35e070
Compare
@danhhz as discussed I have reworked the PR to introduce the notion of "multiple output formats". PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left for format bikeshedding, but everything else looks great!
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/workload/cli/format.go, line 22 at r2 (raw file):
// outputFormat is the interface used to output results incrementally // during a workload run. type outputFormat interface {
👍
pkg/workload/cli/format.go, line 121 at r2 (raw file):
func (f *rawFormatter) outputTick(startElapsed time.Duration, t histogram.Tick) { fmt.Printf("%s %d %.2f %.2f %.2f %.2f %.2f %.2f %s\n",
I understand from our conversation offline that you object to JSON because it's not easily parsed by regex. I am uncomfortable with anything that does not have an easy migration story if we ever need to change what's included here, for example adding p75 or removing a field. For this output, we could append new columns at the end (and deprecating means leaving a placeholder indefinitely), but I wonder if there is a format that meets both our criteria...
How about something like csv with key=value entries? That solves any future delimiter escaping problems we might have, has a straightforward-ish migration story, and is parseable by regex (modulo byzantine examples). I'm also open to other ideas, this one is pretty off the cuff
time=2019-01-01-<...>,errs=0,...p50=1.2,p75=3.4,...,name=foobar
pkg/workload/cli/format_test.go, line 19 at r2 (raw file):
) func Example_text_formatter() {
nice use of example tests!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)
pkg/workload/cli/format.go, line 121 at r2 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
I understand from our conversation offline that you object to JSON because it's not easily parsed by regex. I am uncomfortable with anything that does not have an easy migration story if we ever need to change what's included here, for example adding p75 or removing a field. For this output, we could append new columns at the end (and deprecating means leaving a placeholder indefinitely), but I wonder if there is a format that meets both our criteria...
How about something like csv with key=value entries? That solves any future delimiter escaping problems we might have, has a straightforward-ish migration story, and is parseable by regex (modulo byzantine examples). I'm also open to other ideas, this one is pretty off the cuff
time=2019-01-01-<...>,errs=0,...p50=1.2,p75=3.4,...,name=foobar
I don't object. I'll try it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)
pkg/workload/cli/format.go, line 121 at r2 (raw file):
Previously, knz (kena) wrote…
I don't object. I'll try it out.
I don;'t know if you realized that but your proposal is also valid json (modulo surrounding {} and replacing "=" by ":")
ok, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @knz)
pkg/workload/cli/format.go, line 121 at r3 (raw file):
func (f *jsonFormatter) outputTick(startElapsed time.Duration, t histogram.Tick) { // Note: we use fmt.Printf here instead of json.Marshal to ensure
Hmm, I don't think what you have below is valid json. Don't the field names have to be quoted? (t.Name
also will not be properly escaped, but I care about that less). Why do the extra decimals matter in something designed for consumption by machines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)
pkg/workload/cli/format.go, line 121 at r3 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
Hmm, I don't think what you have below is valid json. Don't the field names have to be quoted? (
t.Name
also will not be properly escaped, but I care about that less). Why do the extra decimals matter in something designed for consumption by machines?
No the field names don't have to be quoted IIRC.
The extra decimals because that's a lot of noise (60% more data to parse, in fact) and it kills the reproducibility of unit tests.
hmm it appears that json does want to quote the property names. It's Javascript and python dicts that don't need that. I decidedly don't like json. |
ok I dropped the idea to use JSON altogether, coming back to some simpler format with explicit column names on each row. RFAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should bite the bullet and use JSON here. It's not perfect, but it's a very common standard and the custom format isn't getting us enough to be worth the overhead of not using something standard (if any other tooling gets built on top of this, they'll have to duplicate your parser wheras they'd very likely get json for free).
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)
Dan I'm going to push back on this
So unless you're volunteering to write the code, I'd like to stick to what I have and say "if we ever need JSON, we can add a new formatter later" |
I don't think adding a new format for each tool that wants to consume this is worthwhile complexity. I think you're overstating the complication of both the parsing regexes and the unit testing. Can you elaborate?
This is not an acceptable way to conduct code reviews. I am in good faith pushing back against what I feel is unnecessary complexity, both in parsing (json is easy, custom formats have lots of gotchas we'll have to work through one by one that json has already solved) and the idea of having a custom output format specifically for your tool. I am open to the idea that I'm missing something. I respect your ability as an engineer and if you're pushing back against what seems obvious to me, there's likely a good reason. But so far, the arguments you've made on this PR and in person haven't really held up |
Show me how to do the unit testing for the format when the float values are all over the place. The |
I haven't tried it, so I don't know where the non-determinism is coming from, but if it's truly unworkable then we can move away from the example tests and do whatever assertions we want in a normal unit test |
Rebased and modified to use JSON as discussed. RFAL |
34e5b37
to
d24d8dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your commit message seems to have staled (still references "incremental-kv")
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz and @knz)
pkg/workload/cli/run.go, line 317 at r4 (raw file):
case "simple": formatter = &textFormatter{} case "incremental-json":
what's incremental about it?
It does not print the final summary |
fixed, thanks |
TFYR! bors r+ |
typo bors r- |
Canceled |
Release note (cli change): The `cockroach workload` command now supports additional command-line parameters to customize the output, in order to facilitate the integration with 3rd party testing tools: - for tools that wish to observe the metrics more frequently than every second, a new flag `--display-every` is now supported, which can be used to specify the period between metric reports. This applies to both the JSON and textual output. - for tools that require a different output format than the default, a new `--display-format` argument is supported. For now only the formats "simple" (original output format) and "incremental-json" (RFC3339 timestamps, no summary row) are supported.
bors r+ |
37929: workload: extend the command-line interface r=knz a=knz Release note (cli change): The `cockroach workload` command now supports additional command-line parameters to customize the output, in order to facilitate the integration with 3rd party testing tools: - for tools that wish to observe the metrics more frequently than every second, a new flag `--display-every` is now supported, which can be used to specify the period between metric reports. This applies to both the JSON and textual output. - for tools that require a different output format than the default, a new `--display-format` argument is supported. For now only the formats "simple" (original output format) and "incremental-json" (RFC3339 timestamps, no summary row) are supported. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
Release note (cli change): The
cockroach workload
command nowsupports additional command-line parameters to customize the output,
in order to facilitate the integration with 3rd party testing tools:
for tools that wish to observe the metrics more frequently than
every second, a new flag
--display-every
is now supported, whichcan be used to specify the period between metric reports.
This applies to both the JSON and textual output.
for tools that require a different output format than the default,
a new
--display-format
argument is supported. For nowonly the formats "simple" (original output format) and
"incremental-json" (RFC3339 timestamps, no summary row)
are supported.