-
Notifications
You must be signed in to change notification settings - Fork 395
Remove pii_safe to remove dynamic messages from logs backend #4985
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
Conversation
Thank you for updating Change log entry section 👏 Visited at: 2025-10-16 20:54:50 UTC |
Typing analysisThis PR does not change typing compared to the base branch. |
BenchmarksBenchmark execution time: 2025-10-21 14:25:15 Comparing candidate commit fb3cbec in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:profiling - Allocations (baseline)
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: fb3cbec | Docs | Was this helpful? Give us feedback! |
I'm unsure how to get Check Pull Request CI Status / all-jobs-are-green (pull_request)Failing after 32m to pass Edit: it has now seemingly passed 🤷 |
a8ae180
to
8984258
Compare
Restating the important part of the document's content:
|
I've added a negative test to ensure exception message content is absent. |
Correct; while there is interpolation, it is essentially from static content:
I've had a hard time tracing it back, but it appears to come from a configuration setting that then maps to specific classes. In all cases it's
Correct; unless the below is overridden in a particular patch implementation, they are essentially static strings from each of the patcher class names: dd-trace-rb/lib/datadog/tracing/contrib/patcher.rb Lines 29 to 31 in c5e3981
To kinda make it explicit and guarantee it via type checking, this signature should probably be:
But that's a bit of rabbit holing for this PR. |
So I believe we discussed introducing new exception types to be emitted by profiling with the fixed strings from C that we could use for this, but this PR doesn't do that. Any plans on still doing that? |
@ivoanjo As far as I'm aware there's:
MY understanding is that this work is out of scope of this PR, which is to be able to restore the currently disabled ingestion stream, and then have work on capturing better details via static strings in subsequent PRs. |
After much effort I managed to reproduce the remaining failure:
This can give us errors like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Left some notes + looking forward to the refactoring that gets us back detailed exception messages from the profiler
This times out:
but this works fine:
I mean, as long as it returns a 200... but there's a
|
Locally I had these
Cleaned up some space, restarted the container with I presume we might be hitting some GHA space limit depending on test ordering (thus seed). |
Messages sent to our backend from the tracing libraries must be constant messages.
This will prevent failing tests from consuming allotted CI budget thus preventing test results to beproperly reported.
Runs after tests have failed (for diagnosis) or succeeded (for comparison) Only free disk space for now, but we could add more.
f40e845
to
c8670d6
Compare
What does this PR do?
Removes the
pii_safe
parameter introduced in PR #4728 to ensure telemetry error logs comply with constant message requirements. We are not supposed to be sending dynamic message content to the logs intake for various reasons (that is what we got agreement to send, we have deduplication logic to limit number of logs, and we don't want any risk whatsoever of sending up PII).Motivation:
Logs have been blocked from being ingested for many months now with exclusion filters.
We'd like to get logs back in ASAP as we want to start proactively resolving errors in
dd-trace-
libraries that are surfaced from these logs as they go into our Error Tracking product.Change log entry
None.
Additional Notes:
I am unfamiliar with
dd-trace-rb
and I asked Claude Code to help with the revert ofpii_safe
, please review this PR as such.This is based off of my document for my findings for Ruby here https://docs.google.com/document/d/16SFeOp7yxzT6dM1O0TKtWkgDsiq6SeV8P_MAYboCdOI/edit?tab=t.o3m3y2eqq0gx
There are a few instances of string interpolation used, I've looked into the flow for the values that they'd be and I think they map to a small subset of known strings from what I can tell, so I am fine overlooking them, but there is risk that we interpolate other future messages:
dd-trace-rb/lib/datadog/di/probe_notifier_worker.rb
Line 277 in c5e3981
dd-trace-rb/lib/datadog/tracing/distributed/propagation.rb
Line 83 in c5e3981
dd-trace-rb/lib/datadog/tracing/contrib/patcher.rb
Line 55 in c5e3981
probe_notifier
appears to be two possibilities:probe status
andsnapshot
propagation
appears to be values such asDatadog::Tracing::Distributed::Propagation::HTTP
(our propagators)patcher
appears to be our integration patchers withindd-trace-rb
so something likeDatadog::Tracing::Contrib::Rails::Patcher
-> there are many of these but seems reasonable IMOHow to test the change?
🤷
I removed / altered tests associated with this parameter so if the current test suite passes then hopefully good.