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.

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

tracingpb: more fine grained redactability for tracing events #103034

Merged
merged 1 commit into from
May 11, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented May 10, 2023

Fixes #103319

Requested by @adityamaru and obliquely suggested by @nvanbenschoten.

Prior to this patch, RecordedSpan and Recording only had a String method to dump a representation, that would remove all redaction markers and turn the result in an unsafe string.

This was making it hard to export recordings into logs while preserving redactability of individual trace spans.

This patch improves the situation by introducing a SafeFormat method which exposes the redactability of trace spans into a mixed string that represents the recording.

Release note: None
Epic: None

Prior to this patch, `RecordedSpan` and `Recording` only had a
`String` method to dump a representation, that would remove all
redaction markers and turn the result in an unsafe string.

This was making it hard to export recordings into logs while
preserving redactability of individual trace spans.

This patch improves the situation by introducing a `SafeFormat` method
which exposes the redactability of trace spans into a mixed string
that represents the recording.

Release note: None
@knz knz requested review from abarganier and a team May 10, 2023 16:21
@blathers-crl
Copy link

blathers-crl bot commented May 10, 2023

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
Copy link
Member

This change is Reviewable

Copy link
Contributor

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: - thanks @knz. I was happy to see that LogRecord.Message is already a redactable string 😎

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@knz knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 11, 2023
@knz
Copy link
Contributor Author

knz commented May 11, 2023

TFYR!

bors r=abarganier

@craig
Copy link
Contributor

craig bot commented May 11, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented May 11, 2023

Build succeeded:

@knz
Copy link
Contributor Author

knz commented May 12, 2023

blathers backport 22.2

@knz knz deleted the 20230510-trace-recordings branch May 12, 2023 10:43
craig bot pushed a commit that referenced this pull request May 22, 2023
103609: roachtest: fix drain test's log searching r=rafiss a=rafiss

c4b6958 changed how the --drain-wait calculation is done, so we must update the test to account for that.

fixes #103577
Release note: None

103716: tracingpb: fix missing return in `Recording.SafeFormat()` r=erikgrinaker a=erikgrinaker

This led to out of bounds panics, e.g.:

```
<empty recording>%!v(PANIC=SafeFormat method: runtime error: index out of range [0] with length 0)
```

Follows #103034.
Touches #103692.

Epic: none
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
craig bot pushed a commit that referenced this pull request May 24, 2023
103223: tracing: fix a bug in the previous commit r=abargainier a=knz

This was a shortcoming in  #103034.

The closing '}' in the representation of OperationMetadata is an unredactable safe string.

I will send a separate PR with a unit test.

Release note: None
Epic: CRDB-27642

Co-authored-by: Raphael 'kena' Poss <[email protected]>
craig bot pushed a commit that referenced this pull request Jun 2, 2023
103180: sql: more redactable details about traces in logs r=abargainier,michae2 a=knz

Followup to #103034.
Fixes #103319.

Prior to this patch, the logging output for SQL statements / txns that exceeded the tracing threshold would mark the entire SQL statement syntax and the entire trace as redactable.

This commit improves the situation by making the statement syntax and the trace more finely redactable, ensuring that more details are included after redaction.

Note: the redaction is still pretty strong by default, as long as `trace.redactable.enabled` defaults to `false`. See #103884.

Release note (sql change): The logging output emitted when `sql.trace.stmt.enable_threshold` or `sql.trace.txn.enable_threshold` are set is now emited on the SQL_EXEC channel instead of DEV as previously.

Epic: CRDB-27642

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing: traces do not have fine-grained redaction
3 participants