lease: unredact lease struct #138800
lease: unredact lease struct #138800
Conversation
aa-joshi
added
backport-23.2.x
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi)
pkg/sql/catalog/lease/descriptor_version_state.go line 39 at r1 (raw file):
// SafeFormat implements redact.SafeFormatter. func (s storedLease) SafeFormat(w redact.SafePrinter, _ rune) {
could you say more about how this fixes it? i did a quick search in the code, and we seem to use the pointer type mostly. also i see above that the String() method uses a pointer receiver.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/descriptor_version_state.go line 39 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could you say more about how this fixes it? i did a quick search in the code, and we seem to use the pointer type mostly. also i see above that the
String()method uses a pointer receiver.
When we are printing the arguments, we are validating the SafeFormat method on object type. Previously, it couldn't find the method. Hence it was getting redacted.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/descriptor_version_state.go line 39 at r1 (raw file):
Previously, aa-joshi (Akshay Joshi) wrote…
When we are printing the arguments, we are validating the SafeFormat method on object type. Previously, it couldn't find the method. Hence it was getting redacted.
Right, but what I am saying is that in all the code I have found, we use the type *storedLease. Can you link me to an area of the code that uses the type storedLease?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/descriptor_version_state.go line 39 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
Right, but what I am saying is that in all the code I have found, we use the type
*storedLease. Can you link me to an area of the code that uses the typestoredLease?
Apologies, I misunderstood. Here it is:
cockroach/pkg/sql/catalog/lease/lease.go
Line 2225 in c8a1890
It was flagged by support team as overly redacted info.
Previously, ID from lease was getting redacted in logs. This was creating challenge to support team to debug issues. This change makes sure that SafeFormat is getting invoked during logging. Epic: CRDB-37533 Part of: CRDB-44885 Release note: None
aa-joshi
force-pushed
the
handle_lease_log
branch
from
a6ea46d to
019a20f
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/descriptor_version_state.go line 39 at r1 (raw file):
Previously, aa-joshi (Akshay Joshi) wrote…
Apologies, I misunderstood. Here it is:
cockroach/pkg/sql/catalog/lease/lease.go
Line 2225 in c8a1890
It was flagged by support team as overly redacted info.
I have updated to pointer as it makes more sense to have it as pointer.
There was a problem hiding this comment.
nice find!
would we want to backport this change? seems like it might help with supportability.
Reviewable status:
|
TFTR! |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 019a20f to blathers/backport-release-23.2-138800: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, ID from lease was getting redacted in logs. This was creating challenge to support team to debug issues. This change makes sure that SafeFormat is getting invoked during logging.
Epic: CRDB-37533
Part of: CRDB-44885
Release note: None