epic-4/story-1: Add Production-Safe Diagnostic Logging - #38
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces centralized, privacy-safe redaction primitives and integrates them into the SDK's logging and error-handling layers. Key changes include the addition of a centralized redaction module, a safe logging wrapper (log_safe), and updated error classes that leverage these primitives. The feedback highlights opportunities to improve robustness, specifically by handling potential ValueError exceptions in urlsplit, explicitly casting visitor_id to a string to prevent runtime crashes, and quoting log values that contain spaces or special characters to preserve log structure integrity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| parts = urlsplit(url) | ||
| host = parts.netloc or "" | ||
| path = parts.path or "" | ||
| if host or path: | ||
| endpoint = f"{host}{path}" | ||
| else: | ||
| # Not a recognizable absolute URL; strip any query suffix from the raw | ||
| # value rather than emitting it verbatim. | ||
| endpoint = url.split("?", 1)[0] |
There was a problem hiding this comment.
Defensive Programming / Robustness: urllib.parse.urlsplit can raise a ValueError if the URL contains invalid/control characters (e.g., under certain Python security updates or malformed inputs). Since redact_url is called during exception formatting (ConfigLoadError, TrackingDeliveryError) and logging (log_safe), any unhandled exception here will crash the error-handling or logging flow, potentially taking down the application or masking the original error. Wrapping urlsplit in a try...except ValueError block with a safe fallback ensures maximum robustness.
try:
parts = urlsplit(url)
host = parts.netloc or ""
path = parts.path or ""
if host or path:
endpoint = f"{host}{path}"
else:
endpoint = url.split("?", 1)[0]
except ValueError:
endpoint = url.split("?", 1)[0]| deterministic (same input → same fingerprint) so diagnostic records for one | ||
| visitor can be correlated without exposing identity. | ||
| """ | ||
| digest = hashlib.sha256((visitor_id or "").encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
Defensive Programming: If a user passes a non-string value (like an integer) as the visitor_id (which is common in Python despite type hints), (visitor_id or "") will evaluate to that non-string object, and calling .encode("utf-8") on it will raise an AttributeError. Converting the value to a string explicitly using str(visitor_id or "") prevents runtime crashes.
| digest = hashlib.sha256((visitor_id or "").encode("utf-8")).hexdigest() | |
| val = str(visitor_id or "") | |
| digest = hashlib.sha256(val.encode("utf-8")).hexdigest() |
| parts = [f"event={event.value}"] | ||
| if context is not None: | ||
| for k, v in context.as_log_fields().items(): | ||
| parts.append(f"{k}={v}") | ||
| for name, value in fields.items(): | ||
| parts.append(f"{name}={_redact_field(name, value)}") | ||
| log.log(level, " ".join(parts)) |
There was a problem hiding this comment.
Robustness / Log Structure Integrity: The log message is constructed by joining key-value pairs with spaces (' '.join(parts)). If any value (such as an experience key, variation key, or custom attribute) contains spaces, tabs, or newlines, it will corrupt the space-separated structure of the log line, making it extremely difficult or impossible for log parsers (like Splunk, Datadog, or ELK) to parse the fields reliably. Quoting values that contain spaces or special characters using json.dumps preserves the integrity of the log structure.
def _quote_value(val: Any) -> str:
import json
s = str(val)
if any(c in s for c in ' \t\n"\\'):
return json.dumps(s)
return s
parts = [f"event={event.value}"]
if context is not None:
for k, v in context.as_log_fields().items():
parts.append(f"{k}={_quote_value(v)}")
for name, value in fields.items():
parts.append(f"{name}={_quote_value(_redact_field(name, value))}")
log.log(level, " ".join(parts))023cd99 to
9598cad
Compare
695fd28 to
a0d368d
Compare
9598cad to
78d7ca6
Compare
…tion.py) Beads: ai-driven-product-dev-wmh7 redact_key/redact_url/SafeContext/fingerprint_visitor; L0 leaf, stdlib only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Beads: ai-driven-product-dev-tf6b Optional[logging.Logger]=None; default resolves convert_sdk namespace logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…redaction.py Beads: ai-driven-product-dev-99y0 Single redaction implementation; ConfigLoadError/TrackingDeliveryError contracts unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Beads: ai-driven-product-dev-xgcy Event-oriented LifecycleEvent-keyed records; structural redaction at all levels; SDKConfig.logger honored; library-logging discipline preserved. Story 2.4 helpers untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + context.py Beads: ai-driven-product-dev-qko1 Additive log_safe emission at init/config-load (core) and bucketing/conversion (context) seams; hashed visitor ref, allowlisted fields only; no outcome change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Beads: ai-driven-product-dev-2y4i Extend integration suite (direct-config + sdk_key qs-06 flows leak nothing) and test_layering.py (_internal/redaction stdlib-only, logging.py no higher-layer imports). Full suite 595 (parity 123 intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…safe kwargs Structurally replace values under visitor_id/attributes/email/name field names with [REDACTED] so a raw PII value can never leak through log_safe even on future misuse. Adds regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a0d368d to
2091406
Compare
F-066 propagation (rebase onto remediated 3-3)Rebased onto the remediated stack (3-3
|
|
Superseded — all commits already in main (bc76b64). Closing without merge as part of post-sprint cleanup. |
Story 4.1 — Add Production-Safe Diagnostic Logging
First story of Epic 4. Adds the production-safe diagnostic logging layer and the centralized redaction primitive frozen by
qs-08-secret-redaction.What was built
_internal/redaction.py(NEW, L0 leaf, stdlib-only):redact_key(sdk_key_abcdef1234567890→sdk_****_7890; short keys fully masked),redact_url(host+path, entire query string elided),SafeContextdataclass (six approved fields),fingerprint_visitor(SHA-256 ref, never rawvisitor_id).logging.py(EXTENDED — Story 2.4 helpers preserved): addedlog_safe()event-oriented wrapper keyed off theLifecycleEventvocabulary; structural NFR6 defense-in-depth that redacts PII-named kwargs.errors.py: Story 1.2 inline redaction shim repointed onto_internal/redaction.redact_url(one redaction implementation; exception public contracts unchanged — enrichment is Story 4.2).core.py/context.py: additive event-oriented diagnostic emission at the init and orchestration seams — allowlisted fields + hashed visitor ref only. No control-flow / readiness / evaluation / tracking behavior change. Determinism untouched.config.py: additivelogger: Optional[logging.Logger] = None.NFR coverage
Tests
Beads
Epic
ai-driven-product-dev-xk4a; tasks-wmh7,-xgcy,-99y0,-tf6b,-qko1,-2y4i— all closed.Sprint / review notes
sprint/2026-04-06-convert-python-sdk. Stacked on epic-3/story-5 (PR epic-3/story-5: Establish JavaScript parity fixtures for state and evaluation behavior #37 → epic-3/story-4: Add entity lookup helpers #36 → epic-3/story-3: Add Default Segments and Custom Segment Evaluation #35 → epic-3/story-2: Support Mutable Visitor State on Contexts #34 → epic-3/story-1: Add the Persistence Boundary and In-Memory Store #33).readiness-assessment.md.log_safekwargs) fixed in round 1, then clean — review-passed, no unresolved warnings.SDKConfig.loggeralready existed (Story 1.2 surface); it was NOT on disk, so it was added additively here. Worth a maintainer glance.🤖 Generated with Claude Code