feat(telemetry): implement secure telemetry logging with PII protection#3948
Conversation
b572220 to
38893c1
Compare
bouwkast
left a comment
bouwkast
left a comment
There was a problem hiding this comment.
Wow this looks great thanks a bunch @kakkoyun
I think this will do it, I don't see anything that stands out.
I wasn't actually aware of WithTags being a thing, but they appear fine to me, I would like to follow up on that outside of this PR with the ASM team (I think they implemented it) to just figure out what the expectations are for that.
(don't take this as a go review 😛)
| } | ||
|
|
||
| func logLocalDiagnosticMessages(name string, feature *libddwaf.Feature) { | ||
| logger := telemetryLog.With(telemetry.WithTags([]string{"appsec_config_key:" + name, "log_type:local::diagnostic"})) |
There was a problem hiding this comment.
I think my understanding is that the WithTags is something that ASM designed / built into the telemetry logs.
I am actually not very familiar with them, but so far I think they are okay, they fall out of the message (I think) so hopefully they don't impact our deduplication logic.
I'll plan on following up on these though to find out more about how they work - it seems that ASM pushed for this and relies on it entirely as some of their logs don't get sent anywhere else.
There was a problem hiding this comment.
Adding clarification here -- the log tags are part of the instrumentation telemetry v2 specification. They are directly sent as tag=value to logs-backend instead of having to avoid having to apply grok parsing backend-side. we are the first to use it in dd-trace-go bc the actual implementation of it is fairly recent but it is fairly used in APM Onboarding even back from an RFC from 2022
There was a problem hiding this comment.
Wow this is a great addition!
|
|
||
| //go:build ruleguard | ||
|
|
||
| // Package gorules contains security-focused telemetry logging rules for dd-trace-go. |
There was a problem hiding this comment.
and an analyzer very nice 🚀 🎉
There was a problem hiding this comment.
Thanks for the work but I believe this something missing in the way this will work. You see with the WithStacktrace option, the actual stacktrace is taken after we know this was a new log message which limits the number of total stacktraces per minute by a lot if a same error happens in every customer http requests.
Side note. With this you can basically delete telemetry.WithStacktrace that would make not-redacted stacktraces and replace it with some lazy computing that would call your stacktrace code only when this is a new log message
| } | ||
|
|
||
| func logLocalDiagnosticMessages(name string, feature *libddwaf.Feature) { | ||
| logger := telemetryLog.With(telemetry.WithTags([]string{"appsec_config_key:" + name, "log_type:local::diagnostic"})) |
There was a problem hiding this comment.
Adding clarification here -- the log tags are part of the instrumentation telemetry v2 specification. They are directly sent as tag=value to logs-backend instead of having to avoid having to apply grok parsing backend-side. we are the first to use it in dd-trace-go bc the actual implementation of it is fairly recent but it is fairly used in APM Onboarding even back from an RFC from 2022
|
Another side note: You may want to put back a logger in the |
ff73471 to
0e0f6dd
Compare
c78168c to
204c621
Compare
a8be574 to
f90ba6d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements secure telemetry logging with comprehensive PII protection for the Datadog Go tracer. The telemetry system now enforces strict security controls through constant-only messages, automatic stack trace redaction, and a new SafeError API that prevents sensitive data exposure in telemetry sent to external services.
- Redesigned telemetry Error API to require constant messages with structured slog.Attr parameters
- Implemented SafeError with automatic stack trace redaction for customer code frames
- Created comprehensive static checker rules to prevent all telemetry security violations
Reviewed Changes
Copilot reviewed 40 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/gencontribs/ | New code generation tool for extracting third-party libraries from contrib/ structure |
| rules/telemetry_rules.go | Comprehensive ruleguard rules enforcing telemetry security policies |
| rules/logging_rules.go | Updated logging rules removing telemetry-specific constraints |
| internal/telemetry/log/ | Complete redesign with secure Logger API using constant messages and SafeError |
| internal/telemetry/backend.go | New telemetry backend with stack trace redaction and message formatting |
| internal/stacktrace/ | Enhanced with customer code redaction, raw capture, and third-party library detection |
| Multiple appsec files | Migration to new secure telemetry logging API throughout codebase |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Tests failed on this commit b143198: What to do next?
|
| // prefixTrie is a thread-safe trie data structure optimized for prefix matching. | ||
| // It's designed for high-performance concurrent read operations with occasional writes. | ||
| // | ||
| // Memory vs Performance Trade-offs: |
There was a problem hiding this comment.
I really like this section, the explanation, and the implementation
Awesome work!
Signed-off-by: Kemal Akkoyun <[email protected]>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
dd-devflow
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: rejected
mergequeue-status: queued
labels
dd-devflow
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
Eliminate PII leakage in telemetry data sent to external services by enforcing
strict security controls on telemetry logging with comprehensive static analysis.
Redesigned telemetry Error API to require constant messages only, implemented
SafeError with automatic stack trace redaction, and created mission-critical
static checker rules with bulletproof scoping.
SafeError extracts error types and redacts customer stack frames using contrib/
directory for frame classification. API breaking change from telemetrylog.Error(err)
to Error("message", SafeError(err)) ensures constant messages with secure error
details.
Signed-off-by: Kemal Akkoyun [email protected]
What does this PR do?
Motivation
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!