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

tracing: enable fine-grained redactability for unstructured events #103884

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented May 25, 2023

Fixes #103883
Epic: CRDB-27642

This enables finer-grain redraction for unstructured events in traces. See #103883 for a discussion. (This can be reverted in case performance is found to be unacceptable.)

Release note: None

@knz knz requested review from dhartunian and a team May 25, 2023 11:58
@blathers-crl
Copy link

blathers-crl bot commented May 25, 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
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


-- commits line 4 at r1:
nit: grained


pkg/util/tracing/tracer.go line 111 at r1 (raw file):

	settings.TenantWritable,
	"trace.redactable.enabled",
	"set to true to enable finer-grain redactability for unstructured events in traces",

nit: grained

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

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


-- commits line 4 at r1:

Previously, dhartunian (David Hartunian) wrote…

nit: grained

Fixed.


pkg/util/tracing/tracer.go line 111 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: grained

Fixed.

@knz knz force-pushed the 20230525-redactable-traces branch from 69dfd05 to 62a0ba0 Compare June 1, 2023 13:19
This enables finer-grained redraction for unstructured events in traces.
See cockroachdb#103883 for a discussion. (This can be reverted in case
performance is found to be unacceptable.)

Release note: None
@knz knz force-pushed the 20230525-redactable-traces branch from 62a0ba0 to 415f82d Compare June 1, 2023 13:36
@knz knz requested a review from a team as a code owner June 1, 2023 13:36
@knz
Copy link
Contributor Author

knz commented Jun 1, 2023

TFYR

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Jun 1, 2023

👎 Rejected by too few approved reviews

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@knz
Copy link
Contributor Author

knz commented Jun 1, 2023

TFYR!

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Jun 1, 2023

Build succeeded:

@craig craig bot merged commit 3dc6fda into cockroachdb:master Jun 1, 2023
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: set trace.redactable.enabled to true by default on the master branch
3 participants