Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tracing: safe formatting Structured trace event redacted in verbose trace #103052

Closed
nvanbenschoten opened this issue May 10, 2023 · 0 comments · Fixed by #103225
Closed

tracing: safe formatting Structured trace event redacted in verbose trace #103052

nvanbenschoten opened this issue May 10, 2023 · 0 comments · Fixed by #103225
Assignees
@knz
Labels
A-observability-inf A-tracing Relating to tracing in CockroachDB. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented May 10, 2023 •
edited by knz
Loading

spanInner.RecordStructured has logic to record structured trace events as unstructured events in verbose traces:

cockroach/pkg/util/tracing/span_inner.go

Lines 268 to 272 in 29523d3

if s.hasVerboseSink() {
// NB: TrimSpace avoids the trailing whitespace generated by the
// protobuf stringers.
s.Record(strings.TrimSpace(item.String()))
}

However, because this logic passes through a raw string, it loses information about redaction and becomes entirely redacted if trace redaction is enabled. This is true even if the original structured event implements the redact.SafeFormatter interface.

Jira issue: CRDB-27814

Epic: CRDB-27642
@nvanbenschoten nvanbenschoten added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-tracing Relating to tracing in CockroachDB. T-observability-inf A-observability-inf labels May 10, 2023
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 10, 2023
@nvanbenschoten
kv: enable trace redaction in concurrency manager data-driven test
d42c6f7 
This commit enables trace redaction in the `TestConcurrencyManagerBasic`
test. Doing so ensures that all logging/tracing performed during concurrency
control is properly redacting sensitive information while preserving all
other information. This ensures that if/when we need these logs in a
customer escalation, they will be there.

In order to make this change, the commit implements `redact.SafeFormatter`
on a few different data types. Notable, it implements the interface on
`waitingState`. With these changes, the only redacted information left is
`roachpb.Key`s and `kvpb.ContentionEvent`s. The latter type should not be
fully redacted, but it is because of cockroachdb#103052.

Epic: None
Release note: None
@nvanbenschoten nvanbenschoten mentioned this issue May 10, 2023
Merged
@knz knz mentioned this issue May 12, 2023
Merged
@knz knz self-assigned this May 12, 2023
@craig craig bot closed this as completed in bd6f8f5 May 24, 2023
craig bot pushed a commit that referenced this issue May 24, 2023
@nvanbenschoten
Merge #103055
ff6a4ac 
103055: kv: enable trace redaction in concurrency manager data-driven test r=knz a=nvanbenschoten

This commit enables trace redaction in the `TestConcurrencyManagerBasic` test. Doing so ensures that all logging/tracing performed during concurrency control is properly redacting sensitive information while preserving all other information. This ensures that if/when we need these logs in a customer escalation, they will be there.

In order to make this change, the commit implements `redact.SafeFormatter` on a few different data types. Notable, it implements the interface on `waitingState`. With these changes, the only redacted information left is `roachpb.Key`s and `kvpb.ContentionEvent`s. The latter type should not be fully redacted, but it is because of #103052.

Epic: CRDB-27642
Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@knz knz

Labels
A-observability-inf A-tracing Relating to tracing in CockroachDB. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tracing: assorted tracing fixes knz/cockroach
2 participants
@knz @nvanbenschoten