Skip to content

Conversation

@mberhault
Copy link
Contributor

@mberhault mberhault commented Jun 21, 2018

This adds basic file stats to the stores report page.
Also improves the styling:

  • show decoded key info protobuf fields rather than raw proto (eg:
    creation date rather than unix timestamp)
  • table styling moved to core style file
  • full-width cells to head different sections

Other changes:

  • use selectors for memoization
  • use loading component

Release note (admin ui change): add encryption statistics on stores report page

@mberhault mberhault requested review from a team June 21, 2018 12:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch 4 times, most recently from 858b728 to c546920 Compare June 22, 2018 11:36
@mberhault
Copy link
Contributor Author

screenshot of the report page styling with two stores (one with --enterprise-encryption, one without):
stores_report

I'm not crazy about the full-span header column, but it's a clearer way of denoting a separate section than alternating background colors.

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from c546920 to 161c454 Compare June 22, 2018 11:45
@mberhault
Copy link
Contributor Author

Note: the first two commits are #26802 and should be reviewed there. This is only the UI changes.

@mberhault mberhault changed the title WIP: ui: encryption stats on stores report ui: encryption stats on stores report Jun 22, 2018
render(): React.ReactElement<any> {
renderKey(isStoreKey: boolean, key: protos.cockroach.ccl.storageccl.engineccl.enginepbccl.KeyInfo$Properties) {
// Get the enum name from its value (eg: "AES128_CTR" for 1).
const encryptionType = protos.cockroach.ccl.storageccl.engineccl.enginepbccl.EncryptionType[key.encryption_type];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vilterp, @BramGruneir: FYI since we spent so much time looking for it: this works too to get the enum's string value. Good thing too because using __proto__ makes ui-lint fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, not sure how we missed this. Glad you found it; __proto__ kind of sketchy (also glad it made the linter fail).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what were you guys trying to do with __proto__???

@bdarnell
Copy link
Contributor

:lgtm:


Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


Comments from Reviewable

Copy link
Contributor

@vilterp vilterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly an improvement over what's currently in master, but could use some major design attention (cc @josueeee), e.g. folding all this info into the node detail page or something and getting it into a more standard table.

Code-wise it seems fine; couple of minor comments and lint errors.

text-align center
color white
font-weight 900

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BramGruneir is there a reason that all the debug reports have their styles in here, whereas top-level pages have their own style files?

);
}

renderStore(store: protos.cockroach.server.serverpb.StoreDetails$Properties, key: number) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to use the Loading component in this file. See jobs/index.tsx for an example.

@BramGruneir
Copy link
Member

pkg/ui/styl/pages/reports.styl, line 162 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

@BramGruneir is there a reason that all the debug reports have their styles in here, whereas top-level pages have their own style files?

Originally, there was a plan for styles for subsections, like reports, to be in their own file. And as the styles started to diverge more from the base style, it made sense to keep it separate.


Comments from Reviewable

import { EncryptionStatusProps } from "oss/src/views/reports/containers/stores/encryption";
import { Bytes } from "src/util/format";

const dateFormat = "Y-MM-DD HH:mm:ss Z";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest omitting the time zone here.

Also maybe leverage the helpers in src/views/reports/containers/range/print (which could probably be moved to src/util)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed time zone.
I doesn't seem range/print handles seconds since epoch, just the proto Timestamp type.

}

render(): React.ReactElement<any> {
renderKey(isStoreKey: boolean, key: protos.cockroach.ccl.storageccl.engineccl.enginepbccl.KeyInfo$Properties) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please no mystery boolean parameters.

at a first glance the two cases seem to share almost nothing. Looking closer, they share an awful lot that's just been copy-pasted. please clean this up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split into separate functions.

render(): React.ReactElement<any> {
renderKey(isStoreKey: boolean, key: protos.cockroach.ccl.storageccl.engineccl.enginepbccl.KeyInfo$Properties) {
// Get the enum name from its value (eg: "AES128_CTR" for 1).
const encryptionType = protos.cockroach.ccl.storageccl.engineccl.enginepbccl.EncryptionType[key.encryption_type];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what were you guys trying to do with __proto__???

}

renderFileStats(stats: protos.cockroach.server.serverpb.StoreDetails$Properties) {
if (stats.total_files.eq(0) && stats.total_bytes.eq(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these and every reference to Longs in protobufs needs to be guarded with FixLong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


let percentFiles = 100;
if (stats.active_key_files !== stats.total_files) {
percentFiles = Long.fromInt(100).mul(stats.active_key_files).toNumber() / stats.total_files.toNumber();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do some arithmetic in one data type and the rest of the arithmetic in a different data type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean do the multiplication using Long and the division using floats? I need longs for multiplications for accuracy, but I need floats for division otherwise I get 33.00% instead of 33.33%

percentFiles = Long.fromInt(100).mul(stats.active_key_files).toNumber() / stats.total_files.toNumber();
}
let fileDetails = percentFiles.toFixed(2) + "%";
fileDetails += " (" + stats.active_key_files + "/" + stats.total_files + ")";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend splitting up the numerical pieces from the formatting pieces so you can reasonably test the math and reasonably see what's going on with the formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added calculatePercentage helper

this.renderSimpleRow("Algorithm", encryptionType),
this.renderSimpleRow("Key ID", key.key_id),
this.renderSimpleRow("Created", createdAt),
this.renderSimpleRow("Source", key.source),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throwing all these details in <pre> may look nice to your l33+ h4xx0r eyes but maybe we want to do something else here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. That was left over from dumping the json representation of the proto, I forgot to drop it when properly formatting the fields.


let decodedStatus;

// Attempt to decode protobuf.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this out of the render so it can be independently tested and memoized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to helper function.

this.renderKey(true, decodedStatus.active_store_key),
this.renderKey(false, decodedStatus.active_data_key),
this.renderFileStats(store),
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these rows ever get put into a table? edit: oh i see now that they do elsewhere. this is a really really weird way to split up these components, with this one not returning legal markup. i'm kind of surprised it doesn't run into lint/tsc errors or react warnings...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. Returning arrays of elements? We seem to do this in quite a few places.

{
_.map(stores.stores, (store, key) => (
this.renderStore(store, key)
_.map(_.sortBy(stores.stores, (store) => store.store_id), (store) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to move the sort into the selector so we're not redoing it needlessly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually know what this means.

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from 161c454 to 9afa36c Compare June 28, 2018 10:09
@couchand
Copy link
Contributor

Reviewed 3 of 4 files at r6, 1 of 1 files at r7.
Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 61 at r3 (raw file):

Previously, mberhault (marc) wrote…

Sure it does. They have the same type and value.

Unfortunately that's not how it works in JavaScript. This checks that they are the same object. Since they're deserialized from a proto I think they won't ever be.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 62 at r3 (raw file):

Previously, mberhault (marc) wrote…

You mean do the multiplication using Long and the division using floats? I need longs for multiplications for accuracy, but I need floats for division otherwise I get 33.00% instead of 33.33%

I agree that if you want the percentage value to not be truncated you need to use a float type, but I don't think I agree that you need the multiplication by 100 to be done with longs "for accuracy"...


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 102 at r3 (raw file):

Previously, mberhault (marc) wrote…

I'm not sure what you mean. Returning arrays of elements? We seem to do this in quite a few places.

We frequently return arrays from internal methods of a component, but I don't think we're returning arrays from the main render method of a component anywhere. Can you clarify the breakdown of components here? Are these rows going to get rendered into multiple different types of tables or something?


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 35 at r7 (raw file):

    // Get the enum name from its value (eg: "AES128_CTR" for 1).
    const encryptionType = protos.cockroach.ccl.storageccl.engineccl.enginepbccl.EncryptionType[key.encryption_type];
    const createdAt = moment.unix(key.creation_time.toNumber()).utc().format(dateFormat);

FixLong


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 97 at r7 (raw file):

      decodedStatus = protos.cockroach.ccl.storageccl.engineccl.enginepbccl.EncryptionStatus.decode(data);
    } catch (e) {
      console.log("Error decoding protobuf: ", e);

Do we want to report this error to the user at all?


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, mberhault (marc) wrote…

I don't actually know what this means.

We don't want to do any unnecessary work in the render cycle, because the philosophy of React is to make render super lightweight. So if we need to do something heavy like sorting, we should at least make sure it's memoized so we don't keep sorting the same array over and over again.

Down below in mapStateToProps you provide selectors to extract the important pieces of the AdminUIState into your components props. In this case the selectors are simple property accesses, but in more complicated cases we'll use the library reselect which provides a simple interface to memoize selectors. Take a look at the mapStateToProps of other components, and see how they use createSelector to compose & memoize selectors.


Comments from Reviewable

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch 5 times, most recently from 873e53c to f4163e2 Compare July 5, 2018 13:24
Copy link
Contributor Author

@mberhault mberhault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 33 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

what were you guys trying to do with __proto__???

We were looking for the enum value -> enum field name mapping and got slightly side-tracked.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 61 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Unfortunately that's not how it works in JavaScript. This checks that they are the same object. Since they're deserialized from a proto I think they won't ever be.

True. I'm comparing Long now.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 62 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I agree that if you want the percentage value to not be truncated you need to use a float type, but I don't think I agree that you need the multiplication by 100 to be done with longs "for accuracy"...

Moved all this to calculatePercentage. It technically does long multiplication followed by float division, but I have to convert at some point so might as well do it there.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 102 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

We frequently return arrays from internal methods of a component, but I don't think we're returning arrays from the main render method of a component anywhere. Can you clarify the breakdown of components here? Are these rows going to get rendered into multiple different types of tables or something?

It was ultimately called from the OSS stores report and inserted directly into the table. Changed it to not have a render method, so it's more of a helper.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 35 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

FixLong

Done.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 97 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

Do we want to report this error to the user at all?

It'd be nice to report the issue somehow. I don't particularly want to dump it into the report itself.


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

We don't want to do any unnecessary work in the render cycle, because the philosophy of React is to make render super lightweight. So if we need to do something heavy like sorting, we should at least make sure it's memoized so we don't keep sorting the same array over and over again.

Down below in mapStateToProps you provide selectors to extract the important pieces of the AdminUIState into your components props. In this case the selectors are simple property accesses, but in more complicated cases we'll use the library reselect which provides a simple interface to memoize selectors. Take a look at the mapStateToProps of other components, and see how they use createSelector to compose & memoize selectors.

Yeah, none of this is clear or standardized within the code base. I'm also not sure it really matter for a list of stores.

Copy link
Contributor

@couchand couchand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is close, but there are still some important questions that we should figure out. Let me know if you want to chat about any of the comments.

Reviewed 3 of 3 files at r9.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 62 at r3 (raw file):

Previously, mberhault (marc) wrote…

Moved all this to calculatePercentage. It technically does long multiplication followed by float division, but I have to convert at some point so might as well do it there.

👍


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 102 at r3 (raw file):

Previously, mberhault (marc) wrote…

It was ultimately called from the OSS stores report and inserted directly into the table. Changed it to not have a render method, so it's more of a helper.

Ok, that makes sense. But you probably want to go all the way and not have it be a React component at all now, right? As it is, it's a weird Frankenstein that says it's a React component, but it doesn't have a render method and it's not used as a component.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 97 at r7 (raw file):

Previously, mberhault (marc) wrote…

It'd be nice to report the issue somehow. I don't particularly want to dump it into the report itself.

Agreed, it probably makes sense to display it above the table in an error message.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 61 at r9 (raw file):

  calculatePercentage(active: Long, total: Long): number {
    if (active === total) {

Still broken here, needs to be .eq


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 64 at r9 (raw file):

      return 100;
    }
    return Long.fromInt(100).mul(active).toNumber() / total.toNumber();

i'd still prefer 100 * (active.toNumber() / total.toNumber()) (or if you're worried about overflow, Long.fromInt(10000).mul(active).div(Long.fromInt(100).mul(total)).toNumber() / 100) but i'm not going to push it too hard, this is fine as-is.


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, mberhault (marc) wrote…

Yeah, none of this is clear or standardized within the code base. I'm also not sure it really matter for a list of stores.

I can understand how it looks like that. We haven't had a chance to go back and clean up every page, but if you look at new pages you will see that we are usually quite vigilant about this. It's really important not to prematurely pessimize React render cycles, because you can quickly get crappy performance through death by a thousand cuts.

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from f4163e2 to 07c9ca2 Compare July 24, 2018 12:01
Copy link
Contributor Author

@mberhault mberhault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 102 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Ok, that makes sense. But you probably want to go all the way and not have it be a React component at all now, right? As it is, it's a weird Frankenstein that says it's a React component, but it doesn't have a render method and it's not used as a component.

Done. Made it a plain object, though the renderX methods still return a ReactElement.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 61 at r9 (raw file):

Previously, couchand (Andrew Couch) wrote…

Still broken here, needs to be .eq

Done.


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I can understand how it looks like that. We haven't had a chance to go back and clean up every page, but if you look at new pages you will see that we are usually quite vigilant about this. It's really important not to prematurely pessimize React render cycles, because you can quickly get crappy performance through death by a thousand cuts.

OK, I've poked at this quite a few times, every time to end up even more confused than before, I've reached the limit of what I can do with copy-pasta and well.. logic. Would someone mind terribly taking it over from here? I think I've addressed all other comments.

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from 07c9ca2 to 73d1215 Compare August 1, 2018 09:52
Copy link
Contributor

@couchand couchand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my changes added this is about ready to go. I think the one outstanding item is error reporting, and then LGTM.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 102 at r3 (raw file):

Previously, mberhault (marc) wrote…

Done. Made it a plain object, though the renderX methods still return a ReactElement.

Ok, it's a little weird, but I'm not going to push too hard.


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 97 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

Agreed, it probably makes sense to display it above the table in an error message.

I think this is the only outstanding comment.


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, mberhault (marc) wrote…

OK, I've poked at this quite a few times, every time to end up even more confused than before, I've reached the limit of what I can do with copy-pasta and well.. logic. Would someone mind terribly taking it over from here? I think I've addressed all other comments.

I've taken care of it in mberhault#2

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from 73d1215 to 393b01e Compare August 6, 2018 11:29
@mberhault
Copy link
Contributor Author

Thanks @couchand, I've integrated your changes. However, there seems to be a problem with the stores.data field. I poked around for a while but couldn't figure out what was wrong. Since I'm back in the office, could we sit down and take a look at it at some point?

@vilterp
Copy link
Contributor

vilterp commented Aug 8, 2018

@couchand is out this week and next but I may be able to help.

@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from 393b01e to 5fb77a1 Compare August 9, 2018 18:23
Copy link
Contributor

@vilterp vilterp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash and LGTM

This adds basic file stats to the stores report page.
Also improves the styling:
- show decoded key info protobuf fields rather than raw proto (eg:
  creation date rather than unix timestamp)
- table styling moved to core style file
- full-width cells to head different sections

Other changes:
- use selectors for memoization
- use loading component

Release note (admin ui change): add encryption statistics on stores report page
@mberhault mberhault force-pushed the marc/file_stats_on_stores_report branch from 5dace8a to 3145049 Compare August 9, 2018 18:51
Copy link
Contributor Author

@mberhault mberhault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ui/ccl/src/views/reports/containers/stores/encryption.tsx, line 97 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think this is the only outstanding comment.

Done. Error is shown inside the table itself.


pkg/ui/src/views/reports/containers/stores/index.tsx, line 123 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I've taken care of it in mberhault#2

Thanks! Grabbed your PR and fixed a few things with vilterp's help.

@mberhault mberhault dismissed couchand’s stale review August 9, 2018 18:53

Couch on vacation. All comments addressed.

@mberhault
Copy link
Contributor Author

Squashed and fixed commit message. Thank you all for the reviews and the help with this.

@mberhault
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Aug 9, 2018
26890: ui: encryption stats on stores report r=mberhault a=mberhault

This adds basic file stats to the stores report page.
Also improves the styling:
- show decoded key info protobuf fields rather than raw proto (eg:
  creation date rather than unix timestamp)
- table styling moved to core style file
- full-width cells to head different sections

Other changes:
- use selectors for memoization
- use loading component

Release note (admin ui change): add encryption statistics on stores report page
 

Co-authored-by: marc <marc@cockroachlabs.com>
@mberhault mberhault mentioned this pull request Aug 9, 2018
29 tasks
@craig
Copy link
Contributor

craig bot commented Aug 9, 2018

Build succeeded

@craig craig bot merged commit 3145049 into cockroachdb:master Aug 9, 2018
@mberhault mberhault deleted the marc/file_stats_on_stores_report branch August 9, 2018 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants