UI: add stores report page with CCL switching #26040
Conversation
|
This change is |
|
Small update: if I drop the separate CCL protos file and build them in the same file as the OSS protos, it works fine. That's kind of expected though, and not really desirable. |
|
Q: sorry I didn't think of this before, but can we put encryption status on the existing node details page, which also has store details? (CCL headaches aside)
|
|
Yeah, I'd rather not. Beyond the encryption status and stats, which will get fairly sizable, I also want to include all the other currently-invisible configuration (path, attributes, etc...). I'm happy to repeat some of the information from the node page to make sure this is comprehensive. That said, any ideas why this is broken? The PR message describes the problem. |
|
Added a commit to generate all protos (CCL and OSS) into This now properly decodes the opaque encryption status field: I'll throw that information into the table a bit more properly and call it for this PR. |
|
hum. getting a test failure though. I'm guessing something about the repeated proto definition and the deepequal in the test make it unhappy? Any thoughts? |
|
Hm, don't know how this is related to your changes, but maybe try calling |
mberhault
force-pushed
the
marc/add_stores_report
branch
from
75779c3
to
9c2fba7
Compare
|
Test fixed (followed the recommended toJSON method), and prettified the protobuf output. The stores page is now readable, just ugly. Better interpretation and display of the fields is up next, but I just want to get this one it so I can wrap up my draft docs and give this to people to play with. Here's a sample store page when encryption is enabled: |
mberhault
changed the title
| const { store } = this.props; | ||
| const rawStatus = store.encryption_status; | ||
|
|
||
| console.log("Attempting protobuf decode"); |
There was a problem hiding this comment.
don't forget to remove this
| <tr className="stores-table__row"> | ||
| <th className="stores-table__cell stores-table__cell--header">{header}</th> | ||
| <td className="stores-table__cell" title={value}><pre>{value}</pre></td> | ||
| </tr> |
There was a problem hiding this comment.
should style this a bit to make it more readable. Add a file encryption.styl next to this tsx file, with contents like this: https://github.com/cockroachdb/cockroach/blob/07e2b77d390874e057ec99758c33167f410144c7/pkg/ui/src/views/cluster/containers/nodeOverview/nodeOverview.styl
Except.stores-table instead of just .table. Also make sure to import the styl file from this tsx file.
There was a problem hiding this comment.
Done, but with a fixed width for th as we end up with two different tables.
mberhault
force-pushed
the
marc/add_stores_report
branch
from
acacbe8
to
7b28af4
Compare
Some rudimentary description of encryption at rest, including full example to: * generate keys * enable encryption * change encryption There is much left to do. The section on the admin UI stores page requires cockroachdb/cockroach#26040
|
Can you take another look? I'd like to get this in by Monday's sha pick for the next alpha. |
|
LGTM — might as well get this merged for the alpha. A couple comments however: If additional stores are going to show up below the first one, I'd make Also, your CSS looks right, but we're only getting the border, not the background colors. @BramGruneir was gonna take a look at this; may know what's wrong. |
|
Thanks. Yeah, I don't really know CSS so I'm afraid I'll have to let someone else debug what's going on there. As for styling, I propose we wait until I'm properly formatting the various fields. For now, I just prettify the jsonified proto but I'll need to split the fields off (and just plain not show some of them). |
|
Reviewed 6 of 11 files at r1, 1 of 3 files at r3. pkg/ui/src/redux/apiReducers.ts, line 237 at r3 (raw file):
You should add one more argument here. Look at the commandQueueReducerObj or the rangeLogReducerObj as examples pkg/ui/src/util/api.spec.ts, line 452 at r3 (raw file):
Why was this needed? pkg/ui/src/views/reports/containers/stores/index.tsx, line 14 at r3 (raw file):
This could use with a loading component. It will allow you to remove some of the error states. And there are is a lot of repeated code in those error states. pkg/ui/src/views/reports/containers/stores/index.tsx, line 16 at r3 (raw file):
No need to define lastError if you just make it: See some of the other debug pages that do this. pkg/ui/src/views/reports/containers/stores/index.tsx, line 48 at r3 (raw file):
I think this and renderStore can be converted to react components easily, but that can be an optimization for later. pkg/ui/src/views/reports/containers/stores/index.tsx, line 136 at r3 (raw file):
See my earlier comment about this. pkg/ui/src/views/reports/containers/stores/stores.styl, line 3 at r3 (raw file):
Why isn't this just another entry in Comments from Reviewable |
|
By the way, feel free to merge and address the issues I brought up in a follow up. Review status: 7 of 12 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful. Comments from Reviewable |
Add a stores report page. The main oddity about this is that one of the fields of the response (`StoreDetails.EncryptionStatus`) is a serialized protobuf only available in CCL code. We implement EncryptionStatus in OSS code (returning nothing) and in CCL code (returning the table contents for encryption details). The encryption status display is very basic (just a prettified proto), but sufficient to at least get basic information. Follow up PRs will do: * create a ccl-protos webpack to mimick the current protos pack * properly display encryption status Release note: add a stores report page, including encryption status.
mberhault
force-pushed
the
marc/add_stores_report
branch
from
7b28af4
to
20655ad
Compare
|
Addressed the ones I understand. The others comments are Greek to me. :( Review status: 6 of 12 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful. pkg/ui/src/redux/apiReducers.ts, line 237 at r3 (raw file): Previously, BramGruneir (Bram Gruneir) wrote…
Done. Apparently I copy/pasted one of the few that didn't have pkg/ui/src/util/api.spec.ts, line 452 at r3 (raw file): Previously, BramGruneir (Bram Gruneir) wrote…
There was a very weird failure where the deep equal returned false. We dumped (and diffed) them to no avail. Comparing the contents was easier. Comments from Reviewable |
|
Thanks all for the guidance getting this through. I'll spend some time in the upcoming weeks improving this page from barebones to useful. No guarantees on the pretty. |
|
bors r+ |
26040: UI: add stores report page with CCL switching r=mberhault a=mberhault Add a stores report page. The main oddity about this is that one of the fields of the response (`StoreDetails.EncryptionStatus`) is a serialized protobuf only available in CCL code. We implement EncryptionStatus in OSS code (returning nothing) and in CCL code (returning the table contents for encryption details). The encryption status display is very basic (just a prettified proto), but sufficient to at least get basic information. Follow up PRs will do: * create a ccl-protos webpack to mimick the current protos pack * properly display encryption status Release note: add a stores report page, including encryption status. Co-authored-by: marc <marc@cockroachlabs.com>
Build succeeded |
PR cockroachdb#26040 introduced a dependency on CCL protobufs in the UI. The build changes in that PR were slightly buggy; specifically, any protobuf that was depended upon by both an OSS and CCL protobuf would be included in the JavaScript bundle twice. This sort of double-inclusion causes subtle problems, like the deepEqual in a unit test that suddenly started failing [0]. To ensure only one copy of each protobuf is included, have the OSS and CCL UI builds depend upon completely independent protobuf DLLs. The OSS protobuf DLL includes only OSS protobufs, of course, while the CCL protobuf DLL includes both the OSS and CCL protobufs. The protobuf compiler, pbjs, takes care of including each dependent protobuf exactly once. To avoid creating a webpack configuration for each distribution and distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js), use webpack's support for parameterized configs. For consistency, collapse webpack.oss.js and webpack.ccl.js into a parameterized config in webpack.app.js. Additionally, deduplicate the Make rules that compile the CCL JavaScript and TypeScript protobufs. The OSS and CCL rules are nearly identical, so they can share a recipe with a little bit of Make magic. [0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452 Release note: None
Some rudimentary description of encryption at rest, including full example to: * generate keys * enable encryption * change encryption There is much left to do. The section on the admin UI stores page requires cockroachdb/cockroach#26040
PR cockroachdb#26040 introduced a dependency on CCL protobufs in the UI. The build changes in that PR were slightly buggy; specifically, any protobuf that was depended upon by both an OSS and CCL protobuf would be included in the JavaScript bundle twice. This sort of double-inclusion causes subtle problems, like the deepEqual in a unit test that suddenly started failing [0]. To ensure only one copy of each protobuf is included, have the OSS and CCL UI builds depend upon completely independent protobuf DLLs. The OSS protobuf DLL includes only OSS protobufs, of course, while the CCL protobuf DLL includes both the OSS and CCL protobufs. The protobuf compiler, pbjs, takes care of including each dependent protobuf exactly once. To avoid creating a webpack configuration for each distribution and distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js), use webpack's support for parameterized configs. For consistency, collapse webpack.oss.js and webpack.ccl.js into a parameterized config in webpack.app.js. Additionally, deduplicate the Make rules that compile the CCL JavaScript and TypeScript protobufs. The OSS and CCL rules are nearly identical, so they can share a recipe with a little bit of Make magic. [0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452 Release note: None
|
@mberhault I have enough questions about #26148 that I'd guess it will take a bit to get in, and it doesn't seem to address one of the build issues identified here. Maybe it's time to back out this change, and then get it in once the build problems are fixed. That way it's not blocking other development. As an aside, I appreciate your desire to get this merged before the alpha release, but I think maybe it was merged prematurely. The PR was only open for a few hours of a regular work day and didn't get the WIP label removed until the end of the day. I do think it would have been useful for Nikhil and myself to have had a chance to look at it before merge. |
|
Yeah, that's fair, though if @benesch can get his build fix in today I don't think we need to bother. |
|
As I mentioned, unfortunately in it's current form #26148 doesn't seem to resolve all the issues. |
PR cockroachdb#26040 introduced a dependency on CCL protobufs in the UI. The build changes in that PR were slightly buggy; specifically, any protobuf that was depended upon by both an OSS and CCL protobuf would be included in the JavaScript bundle twice. This sort of double-inclusion causes subtle problems, like the deepEqual in a unit test that suddenly started failing [0]. To ensure only one copy of each protobuf is included, have the OSS and CCL UI builds depend upon completely independent protobuf DLLs. The OSS protobuf DLL includes only OSS protobufs, of course, while the CCL protobuf DLL includes both the OSS and CCL protobufs. The protobuf compiler, pbjs, takes care of including each dependent protobuf exactly once. To avoid creating a webpack configuration for each distribution and distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js), use webpack's support for parameterized configs. For consistency, collapse webpack.oss.js and webpack.ccl.js into a parameterized config in webpack.app.js. Additionally, deduplicate the Make rules that compile the CCL JavaScript and TypeScript protobufs. The OSS and CCL rules are nearly identical, so they can share a recipe with a little bit of Make magic. [0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452 Release note: None
26148: Makefile,pkg/ui: fix compilation of JS CCL protobufs r=couchand,mberhault a=benesch Main commit message reproduced below, but there are two smaller commits as well. --- PR #26040 introduced a dependency on CCL protobufs in the UI. The build changes in that PR were slightly buggy; specifically, any protobuf that was depended upon by both an OSS and CCL protobuf would be included in the JavaScript bundle twice. This sort of double-inclusion causes subtle problems, like the deepEqual in a unit test that suddenly started failing [0]. To ensure only one copy of each protobuf is included, have the OSS and CCL UI builds depend upon completely independent protobuf DLLs. The OSS protobuf DLL includes only OSS protobufs, of course, while the CCL protobuf DLL includes both the OSS and CCL protobufs. The protobuf compiler, pbjs, takes care of including each dependent protobuf exactly once. To avoid creating a webpack configuration for each distribution and distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js), use webpack's support for parameterized configs. For consistency, collapse webpack.oss.js and webpack.ccl.js into a parameterized config in webpack.app.js. Additionally, deduplicate the Make rules that compile the CCL JavaScript and TypeScript protobufs. The OSS and CCL rules are nearly identical, so they can share a recipe with a little bit of Make magic. [0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452 26329: roachpb/client: remove LegacyRangeLookup and DeprecatedRangeLookupRequest r=nvanbenschoten a=nvanbenschoten Cockroach 2.1 clusters will always support meta2 splits, so the legacy range lookup approach is never allowable. No node in a 2.1 cluster will send DeprecatedRangeLookupRequests either, so the code is no longer needed. The change also removes the `DeprecatedReturnIntents` option on ScanRequests. This option was a predecessor to the READ_UNCOMMITTED read consistency type. It was only used in 1.1, so it no longer needs to be supported in 2.1 clusters. 26368: opt: enable distsql_union, fix some issues r=RaduBerinde a=RaduBerinde #### opt: fix two cases of errors caused by "inner" ORDER BY When the `ORDER BY` adds columns to the scope, we want to remove these columns before set operations, and for subqueries. Release note: None #### opt: fix set operations crash during exec When building a set operation, we use the leftScope which might have an ordering. This ordering must be discarded (it can refer to columns that have been removed, causing a crash when we try to execbuild the query). Release note: None #### opt: split distsql_union and enable for opt The opt version of the planner test has some distsql plan differences; they are mostly due to the optimizer ignoring inner ORDER BY (which the planner doesn't). Release note: None Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Add a stores report page. The main oddity about this is that one of the
fields of the response (
StoreDetails.EncryptionStatus) is a serializedprotobuf only available in CCL code.
We implement EncryptionStatus in OSS code (returning nothing) and in CCL
code (returning the table contents for encryption details).
The encryption status display is very basic (just a prettified proto),
but sufficient to at least get basic information.
Follow up PRs will do:
Release note: add a stores report page, including encryption status.