tracing: assorted tracing fixes #103225
tracing: assorted tracing fixes #103225
Conversation
|
This change is |
knz
changed the title
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, 3 of 3 files at r4, 6 of 6 files at r5, 4 of 4 files at r6.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @abarganier and @nvanbenschoten)
Release note: None
`require.NoError` calls `.Error` which strips redaction markers. For clarity, this patch changes the calls to pass the strings to `t.Fatal` directly. Release note: None
Prior to this patch, the value part of span tags was included in the redactable representation of traces as a safe string. This is generally incorrect: the value part can be populated from arbitrary objects in the code including sensitive values. To fix this, this commit considers the value part to always be a sensitive string. A next iteration of this change would be to define a new protobuf field of type `RedactableString` (new field to ensure cross-version compatibility when traces are transferred over the network) and use that to propagate more fine grained redactability in tag values. The treatment of log tags in the logging package can be used for inspiration. Release note: None
This picks up a fix for a bug that was marking unsafe data incorrectly as safe in trace recordings. Release note: None
This intermediate commit removes the extra white space trim that was performed on structured tracing events, and demonstrates (via unit test success) that it was unnecessary. Release note: None
Prior to this patch, structured items added to verbose traces were improperly converted to an unsafe string prior to recording. This was making the entire structured payload redactable in the recording representation. This patch fixes the situation by delegating the representation to the structured item itself, which may implement a SafeFormat method. Release note: None
There was a problem hiding this comment.
Reviewable status:
-- commits line 24 at r10:
Does this mean we'll no longer see these values in eg SHOW TRACES? How about in query bundles or a collector like Jaeger?
It depends. If trace redaction is enabled, they would be removed yes. But it's not enabled by default. |
|
FYI:
|
|
Very cool, makes sense! |
|
TFYR bors r=dhartunian,RaduBerinde |
|
This PR was included in a batch that timed out, it will be automatically retried |
|
Build succeeded: |
Fixes #103052.
Fixes #103227.
Epic: CRDB-27642
Prior to this patch, structured items added to verbose traces were
improperly converted to an unsafe string prior to recording.
This was making the entire structured payload redactable in the
recording representation.
This patch fixes the situation by delegating the representation
to the structured item itself, which may implement a SafeFormat
method.
This patch also ensures that the JSON copy of structured events
is properly marked as redactable.