Repository navigation
Add debug logging for feature flag onboarding - #12897
ethan-romero-dg wants to merge 9 commits into
Conversation
ethan-romero-dg
added
type: feature
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
ea88d17 to
21bc161
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Runtime behavior remains unverified: the specialist assessed source and existing tests without executing them, and production telemetry was intentionally excluded.
🤖 Bits Code Review · Commit 4fb95d7
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Replace the DiagnosticLogger wrappers with plain SLF4J loggers. Agent-side code logs through the tracer's internal logger, so it needs no isolation wrapper. The provider keeps isolation where an application logging backend is involved: debug diagnostics go through debugQuietly and DebugLoggingHook, which cache DD_TRACE_DEBUG and swallow logging and formatting failures. - Warn at provider construction when DD_TRACE_DEBUG is enabled but the application logger blocks DEBUG for datadog.trace.api.openfeature. - Report flags=missing for Remote Config payloads without a flags member, separately from an empty configuration. - Drop the racy inFlight shutdown field and the duplicate parser summary. - Update the README to match. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
DD_TRACE_DEBUG gates the lifecycle, evaluation, and exposure diagnostics, but a few existing debug messages for rare failures follow only the application logger level. Only debug diagnostics are isolated from logging failures; WARN and ERROR messages are not. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Master now drains pending exposures and makes one final best-effort flush on shutdown instead of interrupting the processor. Update the shutdown messages and README to match, and log exposures dropped after shutdown starts and a shutdown wait that expires before the final delivery returns. Rework the affected tests to inject the backend directly, since route selection moved into the backend factory. Note in Provider that evaluation telemetry opt-out does not apply to local debug diagnostics. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4dbc190 to
92567d1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
More details
Static review found that the diagnostics preserve existing flag-evaluation and exposure-delivery behavior. Human PR discussion supplied no additional correctness concerns.
🤖 Bits Code Review · Commit 92567d1
| queue.offer(event); | ||
| final boolean queued = queue.offer(event); | ||
| if (LOGGER.isDebugEnabled()) { | ||
| LOGGER.debug("Feature Flagging exposure {}", queued ? "queued" : "dropped: queue full"); |
There was a problem hiding this comment.
--- found with local Claude review
P1 — Logging once per exposure on the application thread makes overload worse.
accept() runs on the application thread, and the agent logger writes to System.err synchronously through a PrintStream that takes a lock. The queue (65,536 entries) only fills during an exposure burst. That is exactly when dropped: queue full fires once per event, so every request thread then waits on the stderr lock. The queued line has the same cost for every exposure that isn't cached. With per-user targeting keys the LRU cache keeps evicting entries, so that can be close to once per request.
Suggestion: don't log on the producer path. Count drops (and queued events, if useful) in an AtomicLong, and report dropped=N from the serializer once per flush. Or use RatelimitedLogger.
| buffer.add(event); | ||
| return true; | ||
| } | ||
| LOGGER.debug("Feature Flagging exposure suppressed: duplicate"); |
There was a problem hiding this comment.
--- found with local Claude review
P2 — suppressed: duplicate is logged once per duplicate event.
This runs on the serializer thread, but each stderr write slows draining. Under load the queue then fills sooner, which triggers the dropped: queue full path above. Suggestion: count duplicates and add the count to the existing per-flush debug line.
| } | ||
| try { | ||
| log.debug( | ||
| "Feature Flagging evaluation observed at finallyAfter: flag={}, targetingKey={}, context={}, defaultValue={}, observedValue={}, variant={}, reason={}, errorCode={}, errorMessage={}, metadata={}", |
There was a problem hiding this comment.
--- found with local Claude review
P2 — Each evaluation logs the same raw data twice, which adds up to a flood.
With DD_TRACE_DEBUG=true and the app logger at DEBUG (common when root is at DEBUG in dev or staging), one evaluation produces resolution + finallyAfter + the ExposureLoggingHook suppressed/dispatched line. That is 3 lines, or 5–6 when the exposure isn't cached or the evaluation errors. Both resolution and finallyAfter print the full raw context, default value and resolved value, so large object variants and contexts are written twice. At 10–50 flags per request, that is hundreds of lines per request.
Suggestion: keep only one of the two raw lines (the other could log just flag, variant and reason). Also consider capping the length of raw(...) output.
| final ErrorCode code, | ||
| final String errorMessage, | ||
| final boolean observeFullEvaluationData) { | ||
| DebugLoggingHook.resolutionError(key, code, errorMessage); |
There was a problem hiding this comment.
--- found with local Claude review
P2 — The raw errorMessage is logged before the consent filter runs.
The comment just below says these messages "can echo raw evaluation-context values, so they must never reach any consumer". resolutionError logs errorMessage before that filter applies, even when observeFullEvaluationData is off. App logs are often shipped to log management, so context values (emails, user IDs) would leave the process.
Suggestion: pass the consent-filtered message, or explicitly document in the README that debug mode logs raw PII. The same question applies to the raw context in resolution/finallyAfter.
| final int status = response.code(); | ||
| if (response.isSuccessful()) { | ||
| LOGGER.debug( | ||
| "Feature Flagging agentless configuration poll returned HTTP {}", status); |
There was a problem hiding this comment.
--- found with local Claude review
P3 — The HTTP status is logged twice on every poll.
shouldRetry(Response) runs for every response, so a normal poll logs both poll returned HTTP 200 and poll attempt 1/3 returned HTTP 200. Suggestion: remove one of them.
| public boolean shouldRetry(final Exception exception) { | ||
| if (LOGGER.isDebugEnabled()) { | ||
| LOGGER.debug( | ||
| "Feature Flagging agentless configuration poll attempt {}/{} failed: {}", |
There was a problem hiding this comment.
--- found with local Claude review
P3 — A failed poll logs up to 4 stack traces.
The full stack trace is logged on each attempt here, and again in the final ratelimitedLogger.warn (which logs every time when debug is on). Suggestion: log only the exception class/message per attempt, and keep the stack trace for the final warning.
| : raw(result.getFlagMetadata().asUnmodifiableMap()), | ||
| config == null || config.environment == null ? null : config.environment.name, | ||
| config == null ? null : config.createdAt); | ||
| } catch (Throwable failure) { |
There was a problem hiding this comment.
--- found with local Claude review
P3 — catch (Throwable) also swallows OutOfMemoryError and StackOverflowError.
For example, formatting a huge or deeply nested context could throw one of these. formattingFailed would then try to log again, with the throwable. Consider catch (Exception | LinkageError e) so JVM-level errors still propagate.
| testImplementation(libs.bundles.mockito) | ||
| testImplementation(libs.moshi) | ||
| // OpenFeature uses SLF4J 2; the shared Java 8 test binding targets SLF4J 1.7. | ||
| testImplementation("ch.qos.logback:logback-classic:1.6.5") |
There was a problem hiding this comment.
--- found with local Claude review
P3 — Nit: the version is hardcoded (logback-classic:1.6.5). Prefer adding it to the version catalog (libs), like the other test dependencies.
Summary
Improve feature flag onboarding diagnostics following the SDK logging checklist.
flags=missingwhen a payload has noflagsmember, separately from an empty configuration (flags=0).DD_TRACE_DEBUGand the application's SLF4J DEBUG level, and skip raw-value formatting when either is off. WhenDD_TRACE_DEBUGis on but the application logger blocks DEBUG, the provider logs one WARN naming the missing setting.Validation
At 92567d1, rebased on master
fab3bc8b10:The last commit adapts the exposure shutdown diagnostics, README, and two tests to master's drain-and-flush shutdown.
From an earlier revision, not re-run after the logger simplification: agent/provider jars built, and a Java dogfooding app against local agentless/EVP fixtures returned the expected results for eight evaluations in each run. Provider debug lines: 30 with
DD_TRACE_DEBUG=true, zero with false or unset, while Logback remained at DEBUG.Enablement
Set
DD_TRACE_DEBUG=true(or-Ddd.trace.debug=true) and enable DEBUG for the application'sdatadog.trace.api.openfeaturelogger before constructing the provider.DD_TRACE_DEBUGis read once, when the provider is first constructed; changing it requires restarting the application. Other log levels retain their normal behavior.Known gaps
flagsstill marks the provider ready, and evaluations returnGENERAL. Theflags=missinglog makes this visible; rejecting such payloads is a follow-up.🤖 Generated with Claude Code
FFE reviewer selection
Reviewers: @btthomas, @typotter
Why: Guided and observable onboarding — The PR adds local SDK debug logging for Java OpenFeature provider onboarding, gated on DD_TRACE_DEBUG plus the application's SLF4J DEBUG level, with a WARN when the second… Reviewers come from the matched pod.
🤖 Automated selection.
Selection details
Initiative: project / guided-onboarding
Why this match: The PR adds local SDK debug logging for Java OpenFeature provider onboarding, gated on DD_TRACE_DEBUG plus the application's SLF4J DEBUG level, with a WARN when the second setting is missing. The diff logs provider initialization, readiness, and recovery after configuration returns. It also logs when configuration changes or is unavailable, and Remote Config and agentless configuration receipt (separating flags=missing from flags=0). A new DebugLoggingHook logs evaluation details such as values, defaults, context, and errors. Exposure delivery and shutdown outcomes are logged too, and the README documents how to enable all of this. This matches guided-onboarding: SDK lifecycle and local debug logging that separate 'not observed' from 'failed' across provider readiness, configuration receipt, and evaluation. Exposure transport only gets diagnostic logging, which supports onboarding rather than being transport maintenance.
The PR author was excluded. Existing reviewers and teams were preserved.
Debug or fix a problem
Workflow run · Bits Code session
dry_run=false. Unchanged completed runs are skipped. If the receipt saysneeds_reconciliationor a write outcome is uncertain, ask the workflow owner to reconcile GitHub and the receipt before retrying; do not delete the lock blindly.Evidence and run details
✅ Complete — reviewer coverage verified. The workflow checked pending requests or submitted reviews on the evaluated head.
Generated asynchronously by the FFE reviewer-routing workflow.
Changed paths:
products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java,products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DebugLoggingHook.java,products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/DDEvaluator.java,products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/RemoteConfigServiceImpl.java,products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessConfigurationSource.java,products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/UniversalFlagConfigParser.java,products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java,products/feature-flagging/feature-flagging-api/README.mdEvaluated head:
92567d114008b8d7383cedf2d4628e0425e6165cRegistry revision:
revision:1946aa2a-530e-487a-84a7-83aff44a1089Pinned skill source:
d214cfc65ae154b97d39af5cf32caf48aaec9bf0Instructions: workflow-injected snapshot (no plugin loaded)
Instruction snapshot SHA-256:
f23b2a88c13c0da90e2dd58246a14f257af91e2195c555039bd555b52e6f3516