Skip to content

feat(open_feature): emit server-side EVP flagevaluation#5896

Merged
leoromanovsky merged 46 commits into
masterfrom
leo.romanovsky/ffl-2446-evp-flagevaluation-ruby
Jun 30, 2026
Merged

feat(open_feature): emit server-side EVP flagevaluation#5896
leoromanovsky merged 46 commits into
masterfrom
leo.romanovsky/ffl-2446-evp-flagevaluation-ruby

Conversation

@leoromanovsky

@leoromanovsky leoromanovsky commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Motivation

Customers need Ruby services to report the same server-side feature-flag evaluation signal as the other SDKs, and APM needs a backend-verifiable rollout path instead of SDK-specific local proof. This Ruby contribution adds bounded EVP flagevaluation delivery while preserving the existing OTel metric and exposure paths, which supports cross-SDK approval on smoothness and time-to-land.

The Datadog Ruby tracer continues to support older Ruby runtimes for APM. Datadog Feature Flags through OpenFeature now requires openfeature-sdk 0.5.1 or later, which makes Ruby 3.1 the practical runtime floor for this integration because the upstream OpenFeature Ruby SDK provider hook surface is not available on Ruby 2.5.

Changes

  • Registers FlagEvalEVPHook as a provider hook so the OpenFeature SDK invokes EVP capture with SDK-final EvaluationDetails after hook failures and type validation.
  • Keeps the existing OTel hook and metric path unchanged.
  • Renames the capture hook to FlagEvalEVPHook / flag_eval_evp_hook so it is distinct from FlagEvalMetricsHook.
  • Raises the OpenFeature appraisal floor from 0.3.1 to 0.5.1 for Ruby 3.1+ appraisal gemfiles.
  • Leaves normal APM Ruby runtime support unchanged; Ruby 2.5 users can continue using APM, but cannot use Datadog Feature Flags through OpenFeature without upgrading Ruby.
  • Removes the copied batchedflagevaluations.json schema fixture and avoids validating against duplicated backend schema in this repo.
  • Keeps OpenFeature reason out of the EVP enqueue path, writer, aggregation keys, payload shape, tests, and RBS declarations.
  • Prunes evaluation context before buffering, with deterministic ordering and 256 field / 256 character bounds.
  • Aggregates by schema-visible dimensions only: flag key, variant key, allocation key, runtime-default state, error message, real targeting rule key when available, targeting key, and pruned context.
  • Emits schema-visible error.message, preserves visible variant/allocation in degraded rows, and posts batches to /evp_proxy/v2/api/v2/flagevaluation.
  • Drains the bounded writer queue frequently before the flush cadence so bursts reach aggregation instead of filling the handoff queue.
  • Splits final EVP flagevaluation payloads by encoded uncompressed JSON bytes under the 5 MiB EVP limit.
  • Degrades an oversized full-fidelity row by omitting targeting_key and context; drops and logs only if the degraded row still cannot fit.
  • Updates RBS signatures for the corrected no-reason writer, aggregator, and provider hook contract.

Decisions

  • OpenFeature reason is intentionally not part of EVP payloads or aggregate keys because the worker schema does not accept it.
  • EVP capture uses the OpenFeature provider hook lifecycle instead of direct provider dispatch so the event reflects the SDK-final result, including post-provider type mismatches and hook failure defaults.
  • The provider stamps dd.eval.timestamp_ms into flag_metadata at evaluation entry; the EVP hook uses that timestamp later to preserve first/last-evaluation bounds after SDK finalization.
  • openfeature-sdk 0.5.1 is the minimum OpenFeature dependency for this integration because it is the first line we are willing to support for the provider hook and telemetry surface.
  • Ruby 3.1 is the minimum runtime for Datadog Feature Flags through OpenFeature because the upstream OpenFeature Ruby SDK versions that expose the needed hook surface require Ruby 3.1 or later; this does not change Ruby 2.5 APM support.
  • Degraded buckets retain only schema-visible serialized dimensions; they do not claim OTel-cardinality parity when visible EVP fields differ from OTel metric attributes.
  • Queue overflow and degraded overflow are best-effort drops with counters/logging, not blocking behavior on the user evaluation thread.
  • Payload splitting happens in the background Ruby writer, not the provider evaluation path. The split decision uses encoded uncompressed JSON bytes including the {context, flagEvaluations} wrapper.
  • Hook names follow the cross-SDK architecture split: metrics for the existing OTel counter path, EVP for the flagevaluation path.
flowchart TD
  A[drained aggregated rows] --> B[serialize candidate batch as JSON]
  B --> C{batch <= 5 MiB?}
  C -- yes --> D[post asynchronously through EVP proxy]
  C -- no --> E{current row fits degraded?}
  E -- yes --> F[omit targeting_key and context]
  F --> B
  E -- no --> G[drop, log, count]
Loading

Related PRs

Validation Evidence

Dogfooding App

  • ffe-dogfooding app-ruby was run with local dd-trace-rb artifacts and used Datadog::OpenFeature::NativeEvaluator.
  • Evaluated ffe-dogfooding-string-flag through the Ruby dogfooding app with public-safe targeting keys:
    • ruby-batch-evp-20260623T014728Z-alpha
    • ruby-batch-evp-20260623T014728Z-bravo
    • ruby-batch-evp-20260623T014728Z-charlie
  • Sent 12 identical evaluations per targeting key with no changing evaluation attributes; the final app response returned variant_2, and the backend rows below store variant_2 for the aggregated events.

System Tests

./run.sh +v FEATURE_FLAGGING_AND_EXPERIMENTATION --library ruby -k "Test_FFE_EVP_Flagevaluation"
  • Result: 8 passed, 2630 deselected in 50.74s on Ruby 2.36.0-dev, rails72.
  • Interface evidence from that run:
    • Library proxy and Agent intake both returned 202 for /api/v2/flagevaluation payloads.
    • evp-count-flag produced one aggregated bucket for targeting_key=evp-count-user with variant.key=on, allocation.key=default-allocation, and evaluation_count=5.
    • evp-burst-aggregation-flag produced one aggregated bucket for targeting_key=evp-burst-user with variant.key=on, allocation.key=default-allocation, and evaluation_count=512.
    • evp-degradation-flag produced a degraded aggregated bucket with no targeting_key, variant.key=on, allocation.key=default-allocation, and evaluation_count=2000.

Staging End-To-End

  • Dogfooding ran without the local mock-intake EVP tee/proxy, so the Agent sent EVP traffic through the normal backend path.
  • Retriever staging query used --datacenter us1.staging.dog --customer-auth=skip against the flagevaluation track for the exact targeting keys above.
  • Retriever returned one backend row per targeting key, each with flag.key=ffe-dogfooding-string-flag, variant.key=variant_2, allocation.key=allocation-override-392dd7c149f8, and evaluation_count=12:
    • ruby-batch-evp-20260623T014728Z-alpha: first_evaluation=1782179248384, last_evaluation=1782179248487, timestamp=1782179256172, evaluation_count=12
    • ruby-batch-evp-20260623T014728Z-bravo: first_evaluation=1782179248498, last_evaluation=1782179248595, timestamp=1782179256172, evaluation_count=12
    • ruby-batch-evp-20260623T014728Z-charlie: first_evaluation=1782179248607, last_evaluation=1782179248705, timestamp=1782179256172, evaluation_count=12

- Aggregator spec: canonical_context_key type-differentiation, pruning 256/256
- Two-tier aggregation: full/degraded caps, drop-counted overflow
- runtime_default from nil variant, first/last min/max
- Degraded tier omits targeting_key/context (schema omitempty)
- No Digest::MD5 assertion (reviewer concern #3)
…ations transport spec

- aggregator.rb: two-tier full/degraded aggregation with canonical_context_key
  (sorted type-tagged length-delimited, not a hash digest; reviewer concern #3)
- Context pruning: 256 fields / 256 chars before keying (reviewer concern #1)
- Caps: globalCap=131072/perFlagCap=10000/degradedCap=32768 with drop-counted
  overflow (reviewer concern #8); eval-time min/max (reviewer concern #4)
- Runtime default from nil variant (reviewer concern #5)
- writer.rb: background Thread/SizedQueue(4096) flush every 10s to transport
- transport.rb: FlagevaluationsSpec + build_flagevaluations + send_flag_evaluations
  posting to /evp_proxy/v2/api/v2/flagevaluations with EVP subdomain header
- hooks/flag_eval_evp_hook.rb: finally-stage hook, cheap capture + non-blocking
  enqueue only (reviewer concern #7); reads __dd_allocation_key + dd.eval.timestamp_ms
- component.rb: create_flag_eval_evp_hook wired behind DD_FLAGGING_EVALUATION_COUNTS_ENABLED
  killswitch (default on); builds flagevaluations transport + FlagEvaluation::Writer
- provider.rb: hooks() returns both OTel flag_eval_hook and EVP flag_eval_evp_hook
- flag_eval_hook.rb + flag_eval_metrics.rb: byte-for-byte unchanged (PRES-01)
- provider_spec.rb: updated to stub flag_eval_evp_hook; added killswitch test
…hook lifecycle gap)

The Ruby openfeature-sdk (through at least 0.5.x) does not invoke provider
hooks during flag evaluation — Client#fetch_details calls the provider directly
and returns without running any hook lifecycle. FlagEvalEVPHook#finally was
therefore never called, so no events reached the EVP writer queue and
count=0 at mock-intake.

Fix: call FlagEvalEVPHook#finally directly from Provider#evaluate after
engine.fetch_value returns, using lightweight duck-typed wrappers
(EvpEvalContext, HookContext, HookDetails) that satisfy the hook's interface
without depending on SDK-provided hook context objects.

The hook remains registered via Provider#hooks for forward compatibility with
future SDK versions that do implement the full hook lifecycle. call_evp_hook
is idempotent and best-effort: if the component is absent or the killswitch
is off, it is a no-op; any error is logged at debug level and does not
propagate into the evaluation hot path.

EvaluationContext#attributes gap: ::OpenFeature::SDK::EvaluationContext
exposes #fields (all context fields) and #targeting_key but NOT #attributes
(which the hook reads). EvpEvalContext adapts fields -> attributes, excluding
the targeting_key field to match the hook's expected shape.
…that silently killed EVP emission

Two bugs in FlagEvaluation::Writer prevented the EVP flagevaluation component from
ever loading, causing flag_eval_evp_hook to be nil and no events to reach mock-intake:

1. rescue inside until...end without begin (SyntaxError on Ruby 3.x):
   Ruby only allows `rescue` inside `until...end` if it is wrapped in `begin...end`.
   The file failed to parse on every require, raising SyntaxError which propagated
   through create_flag_eval_evp_hook's `rescue LoadError` guard (which does not
   catch ScriptError subclasses) and crashed the entire OpenFeature component.

2. Wrong require_relative depth for core/utils/time (LoadError if syntax were fixed):
   `require_relative '../../../core/utils/time'` from
   lib/datadog/open_feature/flagevaluation/writer.rb resolves to lib/core/utils/time
   (3 levels up = lib/), not lib/datadog/core/utils/time. The exposures worker uses
   the correct `../../core/utils/time` (2 levels up = lib/datadog/), so writer.rb is
   fixed to match.

Both bugs together meant: SyntaxError on parse -> component init crash -> evp hook nil
-> call_evp_hook guard returns immediately -> zero events queued -> zero EVP payloads.

Add writer_spec.rb with a syntax-load regression guard and an enqueue+flush integration
test (uses allow_any_instance_of to prevent background thread start in tests).
@leoromanovsky leoromanovsky added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jun 12, 2026
@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Jun 12, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 97.59%
Overall Coverage: 90.17% (+0.15%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 597ec11 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jun 12, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-06-27 20:58:58

Comparing candidate commit 240a3b8 in PR branch leo.romanovsky/ffl-2446-evp-flagevaluation-ruby with baseline commit 4d54496 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@leoromanovsky leoromanovsky changed the title [FFL-2446] dd-trace-rb: emit EVP flagevaluation (Phase 2 fan-out) feat(open_feature): emit server-side EVP flagevaluation Jun 14, 2026
@dd-octo-sts dd-octo-sts Bot added the core Involves Datadog core libraries label Jun 14, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

Visited at: 2026-06-24 02:05:59 UTC

@dd-octo-sts

dd-octo-sts Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

Untyped methods

This PR introduces 32 partially typed methods, and clears 2 partially typed methods. It decreases the percentage of typed methods from 65.81% to 65.32% (-0.49%).

Partially typed methods (+32-2)Introduced:
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:69
└── def record: (
          flag_key: ::String,
          variant: ::String?,
          allocation_key: ::String?,
          targeting_key: ::String?,
          eval_time_ms: ::Integer,
          attrs: ::Hash[::String, untyped]?,
          ?error_message: ::String?,
          ?runtime_default: bool?
        ) -> void
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:80
└── def flush_and_reset: () -> ::Hash[::Symbol, untyped]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:82
└── def prune_context: (::Hash[::String, untyped]? attrs) -> ::Hash[::String, untyped]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:84
└── def self.prune_context: (::Hash[::String, untyped]? attrs) -> ::Hash[::String, untyped]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:86
└── def self.flatten_context: (::Hash[::String, untyped]? attrs) -> ::Hash[::String, untyped]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:88
└── def self.flatten_value: (
          ::String prefix,
          untyped value,
          ::Hash[::String, untyped] output,
          ::Hash[::Integer, bool] seen,
          ::Integer depth
        ) -> void
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:96
└── def canonical_context_key: (::Hash[::String, untyped]? attrs) -> ::String
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:100
└── def context_value_bytes: (untyped value) -> ::String
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:104
└── def new_entry: (
          ::Integer evaluation_time_ms,
          runtime_default: bool,
          ?error_message: ::String?,
          ?targeting_key: ::String?,
          ?context_attrs: ::Hash[::String, untyped]?
        ) -> ::Hash[::Symbol, untyped]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:112
└── def observe: (::Hash[::Symbol, untyped] entry, ::Integer evaluation_time_ms) -> void
sig/datadog/open_feature/flag_evaluation/writer.rbs:65
└── def enqueue: (**untyped event) -> void
sig/datadog/open_feature/flag_evaluation/writer.rbs:75
└── def snapshot_context_value: (
          untyped value,
          ::Hash[::Integer, bool] seen,
          ::Integer depth
        ) -> untyped
sig/datadog/open_feature/flag_evaluation/writer.rbs:103
└── def build_events: (::Hash[::Symbol, untyped] snapshot) -> ::Array[::Hash[::String, untyped]]
sig/datadog/open_feature/flag_evaluation/writer.rbs:105
└── def build_event: (
          flag_key: ::String,
          variant: ::String?,
          allocation_key: ::String?,
          targeting_key: ::String?,
          entry: ::Hash[::Symbol, untyped],
          flush_time_ms: ::Integer,
          tier: ::Symbol
        ) -> ::Hash[::String, untyped]
sig/datadog/open_feature/flag_evaluation/writer.rbs:115
└── def send_payload_batches: (::Array[::Hash[::String, untyped]] events) -> void
sig/datadog/open_feature/flag_evaluation/writer.rbs:117
└── def send_payload_batch: (::Array[::Hash[::String, untyped]] events) -> untyped
sig/datadog/open_feature/flag_evaluation/writer.rbs:119
└── def encoded_event_for_payload: (
          ::Hash[::String, untyped] event,
          ::Integer base_payload_size
        ) -> [::Hash[::String, untyped], ::Integer, bool]?
sig/datadog/open_feature/flag_evaluation/writer.rbs:124
└── def encoded_event: (::Hash[::String, untyped] event) -> [::Hash[::String, untyped], ::Integer]
sig/datadog/open_feature/flag_evaluation/writer.rbs:128
└── def degrade_event_for_payload_limit: (
          ::Hash[::String, untyped] event
        ) -> ::Hash[::String, untyped]?
sig/datadog/open_feature/flag_evaluation/writer.rbs:134
└── def event_count: (::Hash[::String, untyped] event) -> ::Integer
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:15
└── def finally: (
          hook_context: untyped,
          evaluation_details: untyped,
          **untyped _opts
        ) -> void
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:23
└── def extract_targeting_key: (untyped evaluation_context) -> ::String?
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:25
└── def extract_attributes: (untyped evaluation_context) -> ::Hash[::String, untyped]
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:27
└── def extract_allocation_key: (untyped evaluation_details) -> ::String?
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:29
└── def extract_error_message: (untyped evaluation_details) -> ::String?
sig/datadog/open_feature/hooks/flag_eval_evp_hook.rbs:31
└── def runtime_default?: (untyped evaluation_details) -> bool
sig/datadog/open_feature/hooks/flag_eval_metrics_hook.rbs:13
└── def finally: (
          hook_context: ::OpenFeature::SDK::Hooks::HookContext,
          evaluation_details: ::OpenFeature::SDK::EvaluationDetails,
          **untyped _opts
        ) -> void
sig/datadog/open_feature/provider.rbs:74
└── def sdk_success_details: (
        (ResolutionDetails | Core::FeatureFlags::ResolutionDetails) result,
        ::Hash[::String, untyped] flag_meta
      ) -> ::OpenFeature::SDK::Provider::ResolutionDetails
sig/datadog/open_feature/provider.rbs:79
└── def sdk_error_details: (
        ::OpenFeature::SDK::Provider::ResolutionDetails::value_t default_value,
        ::String? error_code,
        ::String? error_message,
        ::String? reason,
        ?::Hash[::String, untyped] flag_meta
      ) -> ::OpenFeature::SDK::Provider::ResolutionDetails
sig/datadog/open_feature/provider.rbs:87
└── def build_flag_metadata: (
        (ResolutionDetails | Core::FeatureFlags::ResolutionDetails) result,
        ::Integer eval_time_ms
      ) -> ::Hash[::String, untyped]
sig/datadog/open_feature/transport.rbs:41
└── def send_exposures: (::Hash[::Symbol, untyped] payload) -> Core::Transport::Response
sig/datadog/open_feature/transport.rbs:43
└── def send_flag_evaluations: (::Hash[::String, untyped] payload) -> Core::Transport::Response
Cleared:
sig/datadog/open_feature/hooks/flag_eval_hook.rbs:13
└── def finally: (
          hook_context: ::OpenFeature::SDK::Hooks::HookContext,
          evaluation_details: ::OpenFeature::SDK::EvaluationDetails,
          **untyped _opts
        ) -> void
sig/datadog/open_feature/transport.rbs:28
└── def send_exposures: (::Hash[::Symbol, untyped] payload) -> Core::Transport::Response

Untyped other declarations

This PR introduces 2 partially typed other declarations. It increases the percentage of typed other declarations from 83.0% to 83.46% (+0.46%).

Partially typed other declarations (+2-0)Introduced:
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:51
└── @full: ::Hash[::Array[untyped], ::Hash[::Symbol, untyped]]
sig/datadog/open_feature/flag_evaluation/aggregator.rbs:53
└── @degraded: ::Hash[::Array[untyped], ::Hash[::Symbol, untyped]]

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

Comment thread lib/datadog/open_feature/flag_evaluation/aggregator.rb
Comment thread lib/datadog/open_feature/flagevaluation/aggregator.rb Outdated
Comment thread lib/datadog/open_feature/flagevaluation/writer.rb Outdated
Comment thread lib/datadog/open_feature/flagevaluation/writer.rb Outdated
Comment thread spec/datadog/open_feature/flagevaluation/aggregator_spec.rb Outdated
Comment thread spec/datadog/open_feature/flag_evaluation/aggregator_spec.rb
Comment thread spec/datadog/open_feature/flag_evaluation/aggregator_spec.rb
Comment thread spec/datadog/open_feature/flagevaluation/aggregator_spec.rb Outdated
Comment thread spec/datadog/open_feature/hooks/flag_eval_evp_hook_spec.rb Outdated
Comment thread spec/datadog/open_feature/hooks/flag_eval_evp_hook_spec.rb Outdated
@Strech

Strech commented Jun 25, 2026

Copy link
Copy Markdown
Member

@codex review and find edge-cases, concurrency issues and bugs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 03c721fc17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/datadog/open_feature/flagevaluation/writer.rb Outdated
Comment thread benchmarks/open_feature_flagevaluation.rb
Comment thread lib/datadog/open_feature/flag_evaluation/writer.rb
Comment thread lib/datadog/open_feature/flagevaluation/aggregator.rb Outdated
Comment thread lib/datadog/open_feature/flagevaluation/writer.rb Outdated
Comment thread lib/datadog/open_feature/flagevaluation/writer.rb Outdated
@Strech

Strech commented Jun 25, 2026

Copy link
Copy Markdown
Member

@leoromanovsky Probably you know that, but to explain review delays - it's very hard to review massive PR's with lots of new functionality generated by AI.

Recommendations: smaller context and smaller PR's, we don't have restriction on deliverable and that makes partial-PR's possible.

@leoromanovsky

Copy link
Copy Markdown
Contributor Author

@leoromanovsky Probably you know that, but to explain review delays - it's very hard to review massive PR's with lots of new functionality generated by AI.

Recommendations: smaller context and smaller PR's, we don't have restriction on deliverable and that makes partial-PR's possible.

@Strech Let's discuss that above privately.

I made some improvements to the prefork and shutdown behavior that codex raised concerns about.

The first fix prevents the background writer from spending an entire cycle draining an always-growing queue. Normal drain cycles are now bounded, so the writer can still reach its flush cadence under sustained traffic. Shutdown still uses an exhaustive final drain, and that remains protected by the existing 5-second shutdown timeout.

The second fix snapshots the evaluation context attributes when the event is enqueued. Without this, nested hashes, arrays, or mutable strings could be changed by application code after the flag evaluation, and the background writer could emit the later mutated values instead of the values that existed at evaluation time.

Ran them locally against new system tests (DataDog/system-tests#7226) that I will publish after this lands.

To address the codex feedback, I added benchmarking test coverage, similar to the implementation in the Go tracer (https://github.com/DataDog/dd-trace-go/blob/main/openfeature/flageval_logging_scale_test.go).

@leoromanovsky leoromanovsky requested a review from Strech June 28, 2026 21:29

@Strech Strech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

P.S I'm not convinced tho about naming convention violation, that is not enforced.

Comment thread lib/datadog/open_feature/provider.rb Outdated

@TonyCTHsu TonyCTHsu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I need more time to review this

@TonyCTHsu

Copy link
Copy Markdown
Collaborator

Thanks for the work here — and apologies my initial change-request was terse. To be concrete: I'm holding this because a few things don't meet our quality/maintainability bar yet. This isn't about timing or risk — it's that the Ruby guild reads and maintains this code long after merge, so readability/consistency are part of "done." Several of these were already raised by @Strech / Codex and are still open.

Tests

  • Use instance_double(RealClass) and the real OpenFeature SDK types instead of string-name doubles and Struct stand-ins (spec/.../hooks/flag_eval_evp_hook_spec.rb). String/Struct doubles bypass the over-stubbing/interface checks, so a test can pass against a contract that no longer exists. (raised by @Strech)

Dead code

  • Remove the forwarding-only methods in flagevaluation/writer.rb (no logic, just delegation). (raised by @Strech)

Typing

  • Resolve the new steep:ignore in writer.rb#drain_queue rather than suppressing the checker — let's keep type coverage.

Naming

  • flagevaluation/aggregator.rb uses single-letter locals for meaningful values: e for an entry (collides with the rescued-exception idiom), plus flat, v, n, s. Please give these descriptive names.

Conventions

  • camelCase carried over from the Node/Python implementations appears in comments and RSpec descriptions (globalCap / perFlagCap / degradedCap). Please use snake_case / Ruby naming.
  • Directory lib/datadog/open_feature/flagevaluation/ should be flag_evaluation/ to match the FlagEvaluation module (Ruby convention). The wire/endpoint name /api/v2/flagevaluation is fine on the protocol side but shouldn't leak into the Ruby directory name. (raised by @Strech)
  • Remove the Unicode box-drawing section dividers (# ─── … ───) in the specs

None of this is about being a bottleneck — the path to merge is resolving these, and I'll re-review promptly once they're addressed.

@leoromanovsky

leoromanovsky commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the work here — and apologies my initial change-request was terse. To be concrete: I'm holding this because a few things don't meet our quality/maintainability bar yet. This isn't about timing or risk — it's that the Ruby guild reads and maintains this code long after merge, so readability/consistency are part of "done." Several of these were already raised by @Strech / Codex and are still open.

Tests

  • Use instance_double(RealClass) and the real OpenFeature SDK types instead of string-name doubles and Struct stand-ins (spec/.../hooks/flag_eval_evp_hook_spec.rb). String/Struct doubles bypass the over-stubbing/interface checks, so a test can pass against a contract that no longer exists. (raised by @Strech)

Dead code

  • Remove the forwarding-only methods in flagevaluation/writer.rb (no logic, just delegation). (raised by @Strech)

Typing

  • Resolve the new steep:ignore in writer.rb#drain_queue rather than suppressing the checker — let's keep type coverage.

Naming

  • flagevaluation/aggregator.rb uses single-letter locals for meaningful values: e for an entry (collides with the rescued-exception idiom), plus flat, v, n, s. Please give these descriptive names.

Conventions

  • camelCase carried over from the Node/Python implementations appears in comments and RSpec descriptions (globalCap / perFlagCap / degradedCap). Please use snake_case / Ruby naming.
  • Directory lib/datadog/open_feature/flagevaluation/ should be flag_evaluation/ to match the FlagEvaluation module (Ruby convention). The wire/endpoint name /api/v2/flagevaluation is fine on the protocol side but shouldn't leak into the Ruby directory name. (raised by @Strech)
  • Remove the Unicode box-drawing section dividers (# ─── … ───) in the specs

None of this is about being a bottleneck — the path to merge is resolving these, and I'll re-review promptly once they're addressed.

@TonyCTHsu I'll make these changes to advance this PR. We spoke privately about your team adding an Agents.md file to the openfeature directory this week to codify the standards you used to create this comment as a durable object for our future contributions.

@leoromanovsky leoromanovsky requested a review from TonyCTHsu June 29, 2026 16:44

@TonyCTHsu TonyCTHsu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for working through all of this — the cleanup is a real improvement and I appreciate the turnaround.

Approving. One small non-blocking nits for whenever you next touch this — no need to gate the merge on them:

start_background_thread is still a one-line delegator to perform. It now serves as a clear named seam (init / after-fork / lazy-start, and the mock point in specs), so I'm happy to keep it — just noting it in case you'd intended to inline it.

Nice work getting this over the line.

@leoromanovsky

Copy link
Copy Markdown
Contributor Author

Final e2e verification before merging, thanks for the help!

Local system-tests passed for Ruby EVP flagevaluation:

  ./run.sh +v +l ruby FEATURE_FLAGGING_AND_EXPERIMENTATION ++ -k "Test_FFE_EVP_Flagevaluation" -F tests/ffe/test_flag_eval_evp.py

  Result: 8 passed, 2577 deselected in 63.42s.

  The captured system-test payloads also prove the important high-cardinality behavior:

  evp-degradation-flag events=10001 total=10050 visible=10000 degraded=50

  E2E against staging EVP is also green. I ran ffe-dogfooding app-ruby with the local dd-trace-rb PR checkout installed as datadog-2.37.0.dev, generated 12 evaluations for:

  ruby-final-evp-20260630T173652Z

  Retriever returned 12 queryable flagevaluation rows in EVP, one per targeting key, each with evaluation_count=1 and runtime_default_used=true. The ad hoc flag was not configured in
  staging RC, so these are runtime-default rows, but they prove the Ruby SDK event path lands in EVP and is queryable.

@leoromanovsky leoromanovsky merged commit a3dc880 into master Jun 30, 2026
601 of 603 checks passed
@leoromanovsky leoromanovsky deleted the leo.romanovsky/ffl-2446-evp-flagevaluation-ruby branch June 30, 2026 18:31
@dd-octo-sts dd-octo-sts Bot added this to the 2.37.0 milestone Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos core Involves Datadog core libraries feature Involves a product feature openfeature A new component that provider an ability to configure feature flags

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants