server: include list of altered settings in diagnostics reports

[dt]

Knowing what settings are most useful, and, for those where the values
are not potentially sensitive (i.e. non-strings), what values are common
may be useful in determining if we are exposing useful knobs, should be
turing the defaults further, etc.

Release note (general change): include information about changed settings in diagnostics.

[cockroach-teamcity]

This change is 

[awoods187]

cc @nstewart

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[knz]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/server/updates.go, line 299 at r1 (raw file):

					info.AlteredSettings[name] = setting.String(&s.st.SV)
				default:
					info.AlteredSettings[name] = "<non-default>"

you sure you don't want to print out the raw value here?

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

		"server.time_until_store_dead":             "20s",
		"trace.debug.enable":                       "false",
		"version":                                  "<non-default>",

we absolutely want to report the version in full

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/server/updates.go, line 299 at r1 (raw file):

Previously, knz (kena) wrote…

you sure you don't want to print out the raw value here?

yes, very sure: it could be a string or other sensitive setting.

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, knz (kena) wrote…

we absolutely want to report the version in full

it's an opaque byte string as far as we can tell, so I don't think we can easily.

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, dt (David Taylor) wrote…

it's an opaque byte string as far as we can tell, so I don't think we can easily.

Hmm.. we could say that the value returned by transformer must string to a non-sensitive string, since the only current instance (version) does, and then be okay with reporting .String() here

---

Comments from Reviewable

[bdarnell]

---

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/server/updates.go, line 299 at r1 (raw file):

					info.AlteredSettings[name] = setting.String(&s.st.SV)
				default:
					info.AlteredSettings[name] = "<non-default>"

If I set something to a non-default value and then change it back to the default, do we delete it from system.settings or do we write the default value? If the latter, we could do a little better here and distinguish <default> from <non-default>. (We could also maybe distinguish <empty> from non-empty but I'm not sure if that's useful since the defaults are generally empty)

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, dt (David Taylor) wrote…

it's an opaque byte string as far as we can tell, so I don't think we can easily.

We validate it on SET to ensure it is a supported value, so I think it's fine to report.

---

pkg/server/updates_test.go, line 336 at r1 (raw file):

			`SELECT * FROM _ WHERE (_ = length($1::STRING)) OR (_ = $2)`,
			`SELECT * FROM _ WHERE (_ = _) AND (_ = _)`,
			`SET CLUSTER SETTING _ = _`,

Something to think about in the future: It would be fine to not redact the setting name here, and it might be useful to see how often people change different settings vs setting them once.

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/server/updates.go, line 299 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

If I set something to a non-default value and then change it back to the default, do we delete it from system.settings or do we write the default value? If the latter, we could do a little better here and distinguish <default> from <non-default>. (We could also maybe distinguish <empty> from non-empty but I'm not sure if that's useful since the defaults are generally empty)

We never intentionally write defaults to system.settings (outside of the special migrations): tf you use RESET or the TO DEFAUL syntax, we delete it the row, not write the default.

If you explicitly set it to a value that happens to be the default, we persist your value -- e..g if we later change the default, your explicit preference is still respected.

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

We validate it on SET to ensure it is a supported value, so I think it's fine to report.

Right, I think version is probably fine but I don't think StateMachineSetting is in general, unless we add to the contract of the transformFn that its output must be safe to string and report... we can certainly do that, trivially even, since version is the only instance of StateMachineSetting and already satisfies such a rule, but absent such a rule, I didn't think it was safe to whitelist StateMachineSetting.

---

pkg/server/updates_test.go, line 336 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Something to think about in the future: It would be fine to not redact the setting name here, and it might be useful to see how often people change different settings vs setting them once.

We'd have to teach the scrubber to be a bit smarter -- right now it just rips strings out of the AST, and it would need to be a bit smarter to know these strings are safe. Not impossible, just not quite trivial.

---

Comments from Reviewable

[bdarnell]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Right, I think version is probably fine but I don't think StateMachineSetting is in general, unless we add to the contract of the transformFn that its output must be safe to string and report... we can certainly do that, trivially even, since version is the only instance of StateMachineSetting and already satisfies such a rule, but absent such a rule, I didn't think it was safe to whitelist StateMachineSetting.

Right, I think the only thing we could do is whitelist version by name.

---

Comments from Reviewable

[dt]

Review status: 2 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/server/updates_test.go, line 305 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Right, I think the only thing we could do is whitelist version by name.

Ah, yep, good point, can just do it by name too.

Done.

---

Comments from Reviewable

[knz]

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/server/updates_test.go, line 336 at r1 (raw file):

Previously, dt (David Taylor) wrote…

We'd have to teach the scrubber to be a bit smarter -- right now it just rips strings out of the AST, and it would need to be a bit smarter to know these strings are safe. Not impossible, just not quite trivial.

File an issue for me, I know exactly where to look. I can do this in the bug fix period.

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/server/updates_test.go, line 336 at r1 (raw file):

Previously, knz (kena) wrote…

File an issue for me, I know exactly where to look. I can do this in the bug fix period.

#22723

---

Comments from Reviewable