-
Notifications
You must be signed in to change notification settings - Fork 469
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
Diagnostics reporting and global settings #1336
Conversation
035dae6
to
4f48460
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reporting page looks great!
I might suggest for the unfinished configs page that we skip listing/describing all the individual settings for now and just call out the SHOW ALL syntax as a way to list the ones supported in a given version.
We should also mention that the settings are shared between all nodes and thus it is usually a good idea not to adjust a new setting during a cluster upgrade, while older modes may not handle it -- we attempt to gracefully ignore unknown settings written by other nodes or even unrecognized values for known settings (e.g. If a newer node uses adds an optionC
to an A-or-B setting, that node would allow setting it to C), but it is still recommended to ensure all nodes are upgraded before changing new settings.
6b6c658
to
2314c7a
Compare
Thanks, @dt. I removed explanations of current global settings, and I added some callouts before you change a global setting. PTAL. Will move on to the |
I'm on the side that we might be overcompensating in terms of the detail of information we are sharing. I'm trying to find some other examples of how people do it, but I can't seem to find a good example. Review status: 0 of 3 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
Thanks, @dianasaur323. I personally feel that we're not sharing anything alarming; rather, the details try to clarify that there's no need for alarm at all. But let's get @bdarnell's opinion here. |
2314c7a
to
ed5bc49
Compare
For being a transparent, open-source project, I think this approach aligns with our ethics and does so really tastefully. Really great work on this, @jseldress. Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions. global-configurations.md, line 9 at r1 (raw file):
Because node-level configurations do not apply globally, I would change the architecture of this a little bit to align the content to the title. Instead of talking about the two levels of configuration, I would address cluster-wide configurations primarily and only make a note of the node-level configurations. Alternatively, you could change the title of the page to something like "Node- and Cluster-Level Configurations." Also, re: whatever we end up doing to discuss node-level configs, I would also clarify that the settings are defined by the flags passed to the node through global-configurations.md, line 11 at r1 (raw file):
"via SQL diagnostic-reporting.md, line 9 at r1 (raw file):
Maybe add a link to the Admin UI doc pages here inline or in a note that says something to the effect of, "For information about your cluster's diagnostics, use our Admin UI." Think it would be easy to mistake this page's title for Admin UI-type info. diagnostic-reporting.md, line 15 at r1 (raw file):
"shares anonymized storage and table schema details" diagnostic-reporting.md, line 24 at r1 (raw file):
My slight confusion/apprehension here is if these values uniquely identify my cluster. The diagnostic-reporting.md, line 189 at r1 (raw file):
nit: Would prefer a title like "Opt Out of Diagnostics Reporting" diagnostic-reporting.md, line 197 at r1 (raw file):
s/propigated/propagated Comments from Reviewable |
ed5bc49
to
1148760
Compare
Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. diagnostics-reporting.md, line 9 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. diagnostics-reporting.md, line 15 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. diagnostics-reporting.md, line 24 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Yeah, by diagnostics-reporting.md, line 189 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. diagnostics-reporting.md, line 197 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. I really need to use an automated spellchecker! global-configurations.md, line 9 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. Great feedback. global-configurations.md, line 11 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. Comments from Reviewable |
2420382
to
5c7623c
Compare
Reviewed 4 of 4 files at r2, 2 of 2 files at r3. Comments from Reviewable |
5c7623c
to
504f082
Compare
504f082
to
d976595
Compare
Reviewed 2 of 4 files at r2, 1 of 2 files at r3, 3 of 3 files at r4. diagnostics-reporting.md, line 66 at r4 (raw file):
I would place a period between "about the schema of tables" and "All names and ..." global-configurations.md, line 11 at r1 (raw file): Previously, jseldess wrote…
Mention also that only the root user can modify settings. global-configurations.md, line 69 at r4 (raw file):
I think the responsibility disclaimer is too weak with this phrasing. Perhaps invert the sentence: "Use this configuration mechanism at your own risk. If you think you need to tune these knobs, we encourage you to first [discuss your goals ...]. " _data/sidebar_doc.yml, line 205 at r4 (raw file):
SET CLUSTER SETTING is missing here (and SHOW [ALL] CLUSTER SETTING) below Comments from Reviewable |
d976595
to
c713a64
Compare
Review status: all files reviewed at latest revision, 9 unresolved discussions. diagnostics-reporting.md, line 66 at r4 (raw file): Previously, knz (kena) wrote…
Done. global-configurations.md, line 11 at r1 (raw file): Previously, knz (kena) wrote…
I added the detail about global-configurations.md, line 69 at r4 (raw file): Previously, knz (kena) wrote…
Strengthened the language, styled it as a warning, and listed it first. _data/sidebar_doc.yml, line 205 at r4 (raw file): Previously, knz (kena) wrote…
I didn't create stand-alone pages for these statements but rather covered them in the Comments from Reviewable |
LGTM. I agree with being detailed about what's being collected, and especially with being explicit about the scrubbing being done. Reviewed 1 of 4 files at r2, 1 of 2 files at r5. diagnostics-reporting.md, line 17 at r5 (raw file):
How do we want to handle the fact that this set will change over time? In particular we're close to adding details about query skeletons (I think this will be going in for 1.0). It's great that we're specific about what gets shared, but we want to be clear that we may change this in future releases. diagnostics-reporting.md, line 33 at r5 (raw file):
I think I'd drop all the json examples; they're fairly redundant and make the page much more verbose. Or we could collapse them somehow so you can still get to them if you want, but I don't know if it's worth keeping them around (and up to date) if they won't be shown by default (or maybe it is, since it would be nice to show the effects of the scrubbing, especially for the query stats) On the other hand, it would be nice if we had a link in the admin UI to show a copy of all the uploaded information for your cluster. diagnostics-reporting.md, line 64 at r5 (raw file):
We may want to use the word "structure" here instead of "schema", to emphasize that we're only sending the tables in a skeletal form. diagnostics-reporting.md, line 66 at r5 (raw file):
"No actual table data or table/column names are shared..." global-configurations.md, line 2 at r5 (raw file):
Maybe call this "cluster settings" since that's what the commands say? global-configurations.md, line 28 at r5 (raw file):
Remove this. We're hiding the enterprise.enabled setting for 1.0. Comments from Reviewable |
c713a64
to
8bb0b40
Compare
Review status: 0 of 5 files reviewed at latest revision, 15 unresolved discussions. diagnostics-reporting.md, line 17 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
I've added a note that these details may change in the future. I think we should just strive to update this page when we do change or add things. diagnostics-reporting.md, line 33 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Yeah, I like providing the JSON as proof that we're scrubbing strings. I agree that they make the page more verbose, though, especially the schema example, so I've truncated most of the it away and called out that it's just an excerpt of what table structure data looks like when it gets sent. diagnostics-reporting.md, line 64 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Done. diagnostics-reporting.md, line 66 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Done. global-configurations.md, line 2 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Done. global-configurations.md, line 28 at r5 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Done. This section is commented out for now, but might as well remove this setting now. Comments from Reviewable |
8bb0b40
to
b65887e
Compare
cockroach start
docs.Partly address #1277.
This change is