Skip to content
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

cmd/bbolt: write bench results to stdout #767

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Jun 11, 2024

Follow-up on #765, and work towards #750 and #739.

This PR changes the output of the bench results to stdout rather than stderr for ease of working with the output.

However, I'm unsure if we would consider this a breaking change. If we want to delay this change, I can work around this.

Signed-off-by: Ivan Valdes <ivan@vald.es>
@ivanvc
Copy link
Member Author

ivanvc commented Jun 11, 2024

/cc @tjungblu @ahrtr

@k8s-ci-robot k8s-ci-robot requested review from ahrtr and tjungblu June 11, 2024 03:44
@tjungblu
Copy link
Contributor

I think the reason it has split pipes is to not have the progress output mingled together with the result output. Maybe we should swap those?

As for it being a breaking change, I think that's a rather minor breakage... I'm not even sure if anyone else besides us is using the command.

@ahrtr
Copy link
Member

ahrtr commented Jun 11, 2024

I think the reason it has split pipes is to not have the progress output mingled together with the result output. Maybe we should swap those?

My understanding is that we only print errors and usage to Stderr, all others (including progress and results) should go to Stdout?

@ivanvc
Copy link
Member Author

ivanvc commented Jun 11, 2024

I think the reason it has split pipes is to not have the progress output mingled together with the result output. Maybe we should swap those?

@tjungblu, I'm sorry, I may not have understood you. Are you suggesting sending progress and errors to stdout and the result to stderr?

My understanding is that we only print errors and usage to Stderr, all others (including progress and results) should go to Stdout?

@ahrtr, I don't know if my point of view is biased or opinionated, but it makes sense to me that only the application's actual output goes to stdout.

For example, if I want to pipe or use the results from a bench run, if there's progress report on stdout, I'll need to filter it before I can work with it. i.e.,:

./bbolt bench -gobench-output -write-mode rnd -read-mode seq 2>/dev/null | grep ^Benchmark | tee result.txt
# vs.
./bbolt bench -gobench-output -write-mode rnd -read-mode seq 2>/dev/null | tee result.txt

benchstat result.txt result-old.txt ...

@ahrtr
Copy link
Member

ahrtr commented Jun 11, 2024

only the application's actual output goes to stdout.

For example, if I want to pipe or use the results from a bench run, if there's progress report on stdout, I'll need to filter it before I can work with it. i.e.,:

./bbolt bench -gobench-output -write-mode rnd -read-mode seq 2>/dev/null | grep ^Benchmark | tee result.txt
# vs.
./bbolt bench -gobench-output -write-mode rnd -read-mode seq 2>/dev/null | tee result.txt

benchstat result.txt result-old.txt ...

Sounds good to me.

@tjungblu
Copy link
Contributor

/lgtm

@ahrtr ahrtr merged commit a8b2675 into etcd-io:main Jun 12, 2024
16 checks passed
@ivanvc ivanvc deleted the write-bench-results-to-stdout branch June 12, 2024 17:20
tjungblu added a commit to tjungblu/bbolt that referenced this pull request Aug 5, 2024
backport of etcd-io#767 to fix the missing output on nightly benchmarks

Co-authored-by: Iván Valdés Castillo <iv@nvald.es>
Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants