roachprod: configure logging in roachprod library #74223

healthy-pod · 2021-12-22T21:59:05Z

This patch moves pkg/cmd/roachtest/logger to pkg/roachprod/logger
to be used as a logger by the roachprod library. roachtest and the
roachprod binary are then configured to use/pass instances of logger
as needed by pkg/roachprod.

All calls to fmt.Printf and fmt.Println were removed from
pkg/roachprod except in few cases specific to the roachprod
binary. In the future, using fmt.Printf and fmt.Println in
the library (i.e. pkg/roachprod) should be discouraged.

Closes #73234.

Release note: None

blathers-crl · 2021-12-22T21:59:08Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2021-12-22T21:59:12Z

This change is

blathers-crl · 2021-12-22T23:07:14Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

tbg

Reviewed 152 of 152 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @healthy-pod, and @RaduBerinde)

pkg/cmd/roachprod/main.go, line 891 at r1 (raw file):

	loggerCfg := logger.Config{Stdout: os.Stdout, Stderr: os.Stderr}
	var loggerError error
	roachprodLibraryLogger, loggerError = loggerCfg.NewLogger("/Users/ahmad/desktop/roachprod.log")

hello ahmad :-)

healthy-pod

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @RaduBerinde, and @tbg)

pkg/cmd/roachprod/main.go, line 891 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

hello ahmad :-)

Oops! I will remove it. TFTR

rickystewart · 2021-12-23T19:44:04Z

bors r=tbg

craig · 2021-12-23T21:13:13Z

Merge conflict.

This patch moves `pkg/cmd/roachtest/logger` to `pkg/roachprod/logger` to be used as a logger by the roachprod library. `roachtest` and the roachprod binary are then configured to use/pass instances of `logger` as needed by `pkg/roachprod`. All calls to `fmt.Printf` and `fmt.Println` were removed from `pkg/roachprod` except in few cases specific to the roachprod binary. In the future, using `fmt.Printf` and `fmt.Println` in the library (i.e. `pkg/roachprod`) should be discouraged. Release note: None

rickystewart · 2021-12-23T21:59:43Z

bors r=tbg

craig · 2021-12-23T23:25:50Z

Build succeeded:

GitHub CI (Cockroach)

`roachprod.Get` and `roachprod.Put` behaved differently when it comes to logging of errors: the former would not log errors found while running `scp` if there was a file backing the logger, while the latter would. However, there is no reason for this difference, and it was probably an oversight introduced in an old, really large PR (cockroachdb#74223). This commit makes behavior consistent in both calls and should allow us to see `Get` errors in logs, especially important in `roachtest` failures. Epic: None. Release note: None.

89329: cdcevent: only fetch relevant columns in the rowfetcher r=jayshrivastava a=jayshrivastava Previously, a rowfetcher would be configured to fetch all public columns in a table, including virtual columns. Then, if we only wanted to read columns belonging to a particular family, we would index into this row. This change updates row fetchers to only fetch relevant data, including only the primary key columns and columns in a specified family. This change also updates the row fetcher such that it does not fetch virtual columns. Instead, they are added on by the event descriptor. If the descriptor specifies a column outside of the physical row, the row iterator will fill in a null value upon iteration, representing a virtual column. Release note (enterprise change): When a changefeed is run with the option virtual_columns = "null", the virtual column will be ordered last in each row. Informs: #80976 Epic: none 89781: sql: fix logic test option typos r=mgartner a=mgartner This commit fixes a few tests that incorrectly separated test options with a space instead of a comma. Release note: None 89850: roachprod: log Get failures in roachtests r=srosenberg a=renatolabs `roachprod.Get` and `roachprod.Put` behaved differently when it comes to logging of errors: the former would not log errors found while running `scp` if there was a file backing the logger, while the latter would. However, there is no reason for this difference, and it was probably an oversight introduced in an old, really large PR (#74223). This commit makes behavior consistent in both calls and should allow us to see `Get` errors in logs, especially important in `roachtest` failures. Epic: None. Release note: None. 89867: ui: fix undefined table stats r=maryliag a=maryliag Previously, if no value was being passed as totalCount, the table stats were showing an "undefined value". This commit as a default value as 0 for the total. Fixes #89869 Before <img width="411" alt="Screen Shot 2022-10-11 at 8 43 04 PM" src="https://user-images.githubusercontent.com/1017486/195453624-52ad8c79-95ee-4dda-a703-b7492b7e2fe3.png"> After <img width="281" alt="Screen Shot 2022-10-12 at 5 21 52 PM" src="https://user-images.githubusercontent.com/1017486/195453705-e4fff03c-770f-4f32-8b87-3d26b9b072ff.png"> Release note (bug fix): show correct value on table stats on UI, when there is no values to show. 89919: release: generate archives once, fix checksum format r=celiala a=rail Previously, the release archives were generated multiple times and we ended up uploading different binary content to different cloud providers. Also, the checksum format was missing an extra space, what prevents non coreutils checksum tools from working properly. This change makes the releae process to generate archives only once and fixes the checksum format. Fixes: #89915 Fixes: #89917 Release note: None Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Renato Costa <renato@cockroachlabs.com> Co-authored-by: maryliag <marylia@cockroachlabs.com> Co-authored-by: Rail Aliiev <rail@iqchoice.com>

`roachprod.Get` and `roachprod.Put` behaved differently when it comes to logging of errors: the former would not log errors found while running `scp` if there was a file backing the logger, while the latter would. However, there is no reason for this difference, and it was probably an oversight introduced in an old, really large PR (cockroachdb#74223). This commit makes behavior consistent in both calls and should allow us to see `Get` errors in logs, especially important in `roachtest` failures. Epic: None. Release note: None.

healthy-pod requested review from a team as code owners December 22, 2021 21:59

healthy-pod requested review from otan and removed request for a team December 22, 2021 21:59

blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Dec 22, 2021

rickystewart requested a review from tbg December 22, 2021 22:18

rickystewart assigned andreimatei and unassigned andreimatei Dec 22, 2021

rickystewart requested review from RaduBerinde and andreimatei and removed request for otan December 22, 2021 22:19

healthy-pod force-pushed the roachprod-add-logging branch from 5a68f92 to 51e94c0 Compare December 22, 2021 23:07

tbg approved these changes Dec 23, 2021

View reviewed changes

healthy-pod commented Dec 23, 2021

View reviewed changes

healthy-pod force-pushed the roachprod-add-logging branch 2 times, most recently from 59793b3 to 78ac4e1 Compare December 23, 2021 18:15

rickystewart force-pushed the roachprod-add-logging branch from 78ac4e1 to 21a286f Compare December 23, 2021 21:59

craig bot merged commit 053fe7d into cockroachdb:master Dec 23, 2021

healthy-pod deleted the roachprod-add-logging branch September 20, 2022 23:36

renatolabs mentioned this pull request Oct 12, 2022

roachprod: log Get failures in roachtests #89850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachprod: configure logging in roachprod library #74223

roachprod: configure logging in roachprod library #74223

healthy-pod commented Dec 22, 2021 •

edited by rickystewart

Loading

blathers-crl bot commented Dec 22, 2021

cockroach-teamcity commented Dec 22, 2021

blathers-crl bot commented Dec 22, 2021

tbg left a comment

healthy-pod left a comment

rickystewart commented Dec 23, 2021

craig bot commented Dec 23, 2021

rickystewart commented Dec 23, 2021

craig bot commented Dec 23, 2021

roachprod: configure logging in roachprod library #74223

roachprod: configure logging in roachprod library #74223

Conversation

healthy-pod commented Dec 22, 2021 • edited by rickystewart Loading

blathers-crl bot commented Dec 22, 2021

cockroach-teamcity commented Dec 22, 2021

blathers-crl bot commented Dec 22, 2021

tbg left a comment

Choose a reason for hiding this comment

healthy-pod left a comment

Choose a reason for hiding this comment

rickystewart commented Dec 23, 2021

craig bot commented Dec 23, 2021

rickystewart commented Dec 23, 2021

craig bot commented Dec 23, 2021

healthy-pod commented Dec 22, 2021 •

edited by rickystewart

Loading