Skip to content

Conversation

@biglittlebigben
Copy link
Contributor

@biglittlebigben biglittlebigben commented Nov 6, 2025

Makes sure RPC systems that use the livekit logger wrappers do not log these fields.

This also makes indenting and spaces more consistent by using passing the file through clang-format. Happy to revert this if we want to avoid large cosmetic PRs

These other following changes are suggestions and open for debate:

  • changes the go_package location for logger/options.proto to github.com/livekit/protocol/livekit/logger" since github.com/livekit/protocol/logger already exists and has some non generated go code inside.
  • change the redaction code to use <field_name> as redacted value to match the manual redaction code in egress/redact.go.
  • add a unit test for the egress redaction. The unit test lives in github.com/livekit/protocol/livekit because protocol/livekit/sip.go imports github.com/livekit/protocol/logger, so putting the test in github.com/livekit/protocol/logger would create an import loop.

@changeset-bot
Copy link

changeset-bot bot commented Nov 6, 2025

🦋 Changeset detected

Latest commit: 8a286d3

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "github.com/livekit/protocol" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

logger/proto.go Outdated

if proto.HasExtension(f.Options(), logger.E_Redact) {
e.AddString(k, "<redacted>")
e.AddString(k, fmt.Sprintf("<%s>", f.TextName()))
Copy link
Contributor

@paulwe paulwe Nov 6, 2025

Choose a reason for hiding this comment

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

the name is already the field key in the log and replacing the value with the text "redacted" makes it clear why it's been omitted to readers unfamiliar with the message schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trade off is consistency with the existing logic. I have no strong opinion either way. @frostbyte73 what is your opinion here since I believe you wrote the existing redaction code?

@@ -1,7 +1,7 @@
syntax = "proto3";
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is missing the license header

@biglittlebigben biglittlebigben merged commit 37a4228 into main Nov 12, 2025
5 checks passed
@biglittlebigben biglittlebigben deleted the benjamin/redact_proto branch November 12, 2025 21:44
This was referenced Nov 12, 2025
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.

3 participants