util/log: allow bool fields to be emitted as false in json format

[xinhaoz]

Previously with the json logging format, it was not possible to emit boolean fields that were false. Even if the boolean field is marked as 'includeempty', it will be emitted as field: true in the event. While always not including false boolean fields is more space efficient, with certain fields it's helpful to have the field emitted in all instances and explicitly state its 'false' value.

This patch makes it possible for fieldName: false to be emitted in the json logging format when the field is marked as includeempty.

Epic: none
Part of: #108284

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[xinhaoz]

Note that while while the previous behaviour is technically a bug (any boolean fields that were marked as 'includemepty' would always show as true, even if the value is false), it is not a user visible bug because we don't actually have any boolean fields at this point that are marked as 'includemempty'. At this time, all bool fields are simply omitted if they are false.

[maryliag]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @xinhaoz)

---

pkg/util/log/eventpb/eventpbgen/gen.go line 599 at r1 (raw file):

     if printComma { b = append(b, ',')}; printComma = true
     b = append(b, "\"{{.FieldName}}\":"...)
     b = strconv.AppendBool(b, m.{{.FieldName}})

is there any test that can be done to check for other types? just in case this is also happening in other cases

[xinhaoz]

pkg/util/log/eventpb/eventpbgen/gen.go line 599 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

is there any test that can be done to check for other types? just in case this is also happening in other cases

I haven't seen any testing specifically for the code generation and we don't currently have boolean any fields in existing protobufs that are marked as includeempty to validate :/. @abarganier did I miss anything in terms of existing testing? Should I create a testing protobuf with that field type right now and test emitting that?

[xinhaoz]

pkg/util/log/eventpb/eventpbgen/gen.go line 599 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I haven't seen any testing specifically for the code generation and we don't currently have boolean any fields in existing protobufs that are marked as includeempty to validate :/. @abarganier did I miss anything in terms of existing testing? Should I create a testing protobuf with that field type right now and test emitting that?

Ah sorry, your question was specifically for other types. We seem to be using includeempty very sparingly in our protobufs already. I didn't see any other cases of types exhibiting this behaviour in gen.go, I think it was easier for this case specifically because there are only 2 values for a boolean, I'd imagine it'd be more obvious if other types were hard coding a value or had special behaviour for the zero value. In any case, q about testing remains.

[maryliag]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier)

[xinhaoz]

TFTR! Let me know if you have any remaining comments on testing the code generation!
bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)