roachprod-microbench: change sheet order #110448
Conversation
Previously metric units in the summary sheet, and on sheet tabs, were sorted alphabetically. Since `sec/op` is the most important metric it should be first. This change reverses the order to ensure it is. Epic: None Release note: None
herkolategan
requested review from
rachitgsrivastava and
DarrylWong
and removed request for
a team
|
This change is |
| @@ -84,12 +84,13 @@ func (srv *Service) CreateSheet( | |||
| var s sheets.Spreadsheet | |||
| s.Properties = &sheets.SpreadsheetProperties{Title: name} | |||
|
|
|||
| // Sort sheets by name. | |||
| // Sort sheets by name in reverse order. This ensures `sec/op` is the first | |||
| // sheet and metric in the summary. | |||
There was a problem hiding this comment.
I think I see how this works, but if we add a new sheet (watts/sec) this will quietly break. Are we ok with that? My understanding is that we are since we will eventually be looking at Prometheus as source of truth.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @rachitgsrivastava, and @renatolabs)
pkg/cmd/roachprod-microbench/google/service.go line 88 at r1 (raw file):
Previously, renatolabs (Renato Costa) wrote…
I think I see how this works, but if we add a new sheet (
watts/sec) this will quietly break. Are we ok with that? My understanding is that we are since we will eventually be looking at Prometheus as source of truth.
Correct, z-a order doesn't necessarily mean we'll get the "most important" first especially if custom metrics exist. Alternative might be to try and preserve the original ordering.
|
bors r=renatolabs |
|
Build failed: |
|
bors r=renatolabs |
|
Build failed: |
|
The upgrade acceptance test failed by finding a known issue in an older release -- see also #110702. I'll put a patch for it later today. Unrelated to this PR. bors retry |
|
Build succeeded: |
Previously metric units in the summary sheet, and on sheet tabs, were sorted alphabetically. Since
sec/opis the most important metric it should be first. This change reverses the order to ensure it is.Epic: None
Release note: None