Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datadog.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Gem::Specification.new do |spec|
.select { |fn| File.file?(fn) } # We don't want directories, only files
.reject { |fn| fn.end_with?('.so', '.bundle') } # Exclude local profiler binary artifacts
.reject { |fn| fn.end_with?('skipped_reason.txt') } # Generated by profiler; should never be distributed
.reject { |fn| File.basename(fn) == 'AGENTS.md' } # Developer tooling; not useful to gem consumers

spec.executables = ['ddprofrb']
spec.require_paths = ['lib']
Expand Down
385 changes: 385 additions & 0 deletions lib/datadog/open_feature/AGENTS.md
Comment thread
TonyCTHsu marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,385 @@
# OpenFeature coding guide
Comment thread
TonyCTHsu marked this conversation as resolved.

This guide applies to contributors and their AI coding tools working under
`lib/datadog/open_feature/`, `spec/datadog/open_feature/`, and `sig/datadog/open_feature/`.
The goal is to keep review conversations focused on design and correctness rather than
conventions. Each rule is grounded in a real finding from this codebase; the guide grows as
new patterns emerge. When in doubt, follow the existing files in this directory as the
reference.
Comment thread
TonyCTHsu marked this conversation as resolved.

---

## Naming

Ruby values intention-revealing names. Every local variable, method, and parameter should say
what it holds or does.

**Variables: use full, descriptive names.**

```ruby
# bad
flat = flatten_context(attrs)
e = entries.first
s = build_string(parts)

# good
flattened_context = flatten_context(attrs)
first_entry = entries.first
encoded_string = build_string(parts)
```

Single-letter names are acceptable only for established Ruby idioms:

```ruby
# fine: block index
parts.each_with_index { |part, i| ... }

# fine: rescue variable
rescue => e
logger.debug(e.message)

# fine: throwaway / ignored argument
def handle(event, _context)
```

**Directories and modules must match.** A module named `FlagEvaluation` lives in
`flag_evaluation/`, not `flagevaluation/`. Wire-level names (API endpoints, protocol fields)
stay on the protocol side and do not leak into Ruby identifiers.

**Use Ruby naming throughout, including comments and test descriptions.** Do not carry
camelCase from other languages:

```ruby
# bad (in comment or RSpec description)
# globalCap controls the upper bound

# good
# global_cap controls the upper bound
```

---

## Comments

Comment the *why*, not the *what*. Use the minimum words needed to explain the constraint,
decision, or non-obvious behaviour. If a comment keeps growing, that is a sign the code
itself needs to be clearer — extract a method or rename a variable instead.

```ruby
# bad: narrates obvious code across multiple lines
# We iterate over the context attributes and increment the counter for each
# one we process, stopping once we reach the maximum allowed field count.
count = 0
attrs.each { |k, v| count += 1 }

# bad: one-liner that still just restates the code
# increment the retry count
retry_count += 1

# good: one line, explains a non-obvious constraint
# Cap at 256 to match the backend's field limit; extras are silently dropped.
pruned[key] = value if pruned.size < MAX_CONTEXT_FIELDS
```

Do not restate a test description as a comment inside the example:

```ruby
# bad
it "returns nil when the flag is missing" do
# returns nil when the flag is missing
expect(subject).to be_nil
end

# good
it "returns nil when the flag is missing" do
expect(subject).to be_nil
end
```

Do not include references that only make sense outside this repository (ticket IDs, "Node
reference", "Python sibling"). The Ruby code stands alone.

Use ASCII characters only. Do not use Unicode box-drawing dividers (`# ─── Section ───`);
they are not used elsewhere in the codebase.

---

## Methods

Keep methods small and single-purpose. Prefer many focused private methods over one large one.

```ruby
# bad: one method doing too much
def process(event)
key = [event[:flag], event[:variant]].join(":")
return if @seen.include?(key)
@seen << key
payload = { flag: event[:flag], count: (@counts[key] || 0) + 1 }
@transport.send(payload)
end

# good: each step named and separated
def process(event)
key = cache_key(event)
return if already_seen?(key)
record(key)
Comment thread
TonyCTHsu marked this conversation as resolved.
@transport.send(build_payload(key))
end
```

Prefer `return unless condition` over `return nil unless condition` (Ruby implicitly returns
nil):

```ruby
# bad
return nil unless enabled?

# good
return unless enabled?
```

---

## Error handling

Rescue only what you intend to handle, and only as broadly as necessary.

```ruby
# bad: catches everything, hides real bugs
begin
do_work
rescue => e
logger.debug(e.message)
end

# good: deliberate, narrow, explained
@queue.push(event, true)
rescue ThreadError
# Queue full. Drop and count; backpressure is reported on the next flush.
@overflow_count += 1
```

A broad `rescue => e` at a product boundary (flag evaluation, trace pipeline) is acceptable
when the intent is to never interrupt the caller. Scope it tightly and comment why.

---

## Types (RBS)

Every file in `lib/datadog/open_feature/` has a matching signature in
`sig/datadog/open_feature/`. Add or update the `.rbs` file in the same change as the Ruby
file. CI enforces this.

Do not suppress the type checker to make an error go away:

```ruby
# bad
result = compute_value # steep:ignore

# good: fix the type or add an explicit annotation
result = compute_value #: String
```

If a suppression is genuinely unavoidable, add a comment explaining why and call it out in
the PR description. Prefer `Type?` over `(nil | Type)`.

---

## Tests

**Use verifying doubles.** `instance_double(RealClass)` raises when you stub a method that
does not exist on the real class. String-name doubles and hand-rolled `Struct` fakes do not.

```ruby
# bad: passes even if Writer#enqueue is renamed or removed
let(:writer) { double("Writer", enqueue: nil) }
let(:writer) { Struct.new(:enqueue).new(nil) }

# good: fails fast when the interface changes
let(:writer) { instance_double(Datadog::OpenFeature::FlagEvaluation::Writer, enqueue: nil) }
```

**Tests must be independent.** Each example must set up its own state and clean up after
itself. Do not rely on test ordering or on state left by a previous example. This matters
especially when a global singleton (like `OpenFeature::API.instance`) is involved: reset it
in `before`/`after` hooks for every example that touches it, and stub every message an object
will receive.

**Assert exact values when you know them.**

```ruby
# bad: passes even if count is wildly wrong
expect(result.count).to be >= 1

# good
expect(result.count).to eq(3)
```

---

## Concurrency and threads

Background threads have a few mandatory properties in this codebase:

- **Fork safety.** When a process forks (e.g. Puma spawning workers), all background threads
from the parent process die silently in the child — the child starts with no threads even
though the objects still exist. Use `Core::Workers::Async::Thread` with
`FORK_POLICY_RESTART`: it detects that the process was forked and restarts the thread
automatically on the next operation.

- **Bounded queues.** Use `SizedQueue` with an explicit capacity rather than a plain `Array`
or `Queue`. An unbounded queue accepts events faster than they can be flushed; under
sustained load it grows until the process runs out of memory. A `SizedQueue` caps growth
and lets the producer handle the overflow explicitly.

```ruby
# bad: grows without limit
@queue = Queue.new

# good: caps at 4096 entries; producer handles ThreadError on overflow
@queue = SizedQueue.new(4096)
```

- **Shutdown timeout.** Always call `join(timeout)` when stopping the thread. Without a
timeout, a stuck thread prevents the process from exiting.

- **Non-blocking enqueue.** The hook runs on the caller's flag-evaluation thread. Never let
a full queue or a stopped worker stall that thread. Push non-blocking, catch the overflow,
and count the drop — do not raise or wait:

```ruby
# bad: blocks the caller if the queue is full
@queue.push(event)

# good: drops and counts on overflow, never blocks the caller
@queue.push(event, true)
rescue ThreadError
@overflow_count += 1
```

---

## Repo idioms

These apply across the whole repository, not just this directory.

**Time.** Use `Datadog::Core::Utils::Time.now` / `.get_time` instead of `Time.now`. The time
provider is injectable (used by tests and Timecop integrations).

```ruby
# bad
timestamp = Time.now.to_i

# good
timestamp = Core::Utils::Time.get_time
```

**Environment variables.** Read through `Datadog::Core::Environment::VariableHelpers` or the
settings layer, never `ENV` directly. Run `rake local_config_map:generate` after adding a new
environment variable.

**`filter_map`.** Use `Core::Utils::EnumerableCompat.filter_map` instead of the native
`filter_map`. Native `filter_map` requires Ruby 2.7+; this codebase supports Ruby 2.5 and
2.6.

**Settings and component wiring.** New configuration belongs in `Configuration::Settings`
extended via `Core::Configuration::Settings.extend`. New components are built in
`Component.build` and wired into `components.rb`. Follow the existing wiring pattern rather
than inventing a new one.

**Accessing private members.** `.send(:member)` is an accepted pattern here when no public
accessor exists — the tracer uses it in several places. Prefer adding a real public accessor
or seam when one is practical.

---

## Structure and PR hygiene

- One class or module per file, matching the file name.
- Keep PRs focused. Each PR should change one thing: a new feature, a refactor, or a bug
fix — not all three at once. Mixing concerns makes it hard to understand intent, hard to
revert, and slow to review.
- No dead code. Dead code is any method, branch, or class that no current caller reaches.
Common forms: a method defined but never called anywhere; a `rescue` branch for an
exception the surrounding code cannot raise; an `if` branch whose condition is always
false; a "forward compatibility" path added for a future caller that does not exist yet.
Delete it — version control remembers it if it is ever needed again.

```ruby
# dead: start_worker is defined but perform already starts the thread;
# nothing calls start_worker
def start_worker
perform
end

# dead branch: the SDK never dispatches :finally today, so this branch
# is never reached
def run_hook(stage, context)
case stage
when :before then before(context)
when :finally then finally(context) # no caller reaches here
end
end
```

- Always construct objects through their normal constructor. Do not use `.allocate` +
`instance_variable_set` to bypass `initialize` — in tests, benchmarks, or anywhere else.
If the constructor requires collaborators that are hard to supply, pass lightweight
test doubles through the constructor arguments instead:

```ruby
# bad: bypasses initialize, breaks silently when the constructor changes
writer = Writer.allocate
writer.instance_variable_set(:@transport, NoopTransport.new)
writer.instance_variable_set(:@logger, logger)

# good: uses the real constructor, passes a lightweight stand-in
writer = Writer.new(transport: NoopTransport.new, logger: logger)
```

- **PR size.** If the resulting diff would exceed roughly 1000 lines of additions, stop and
warn the contributor before generating the code. Propose a split into smaller, stackable
PRs instead. Each PR should be reviewable on its own and mergeable independently. Get
agreement on the breakdown before generating any code.

### PR description

Use `.github/PULL_REQUEST_TEMPLATE.md` as the structure. Fill each section with one or two
sentences — high-level intent, not a list of every changed file. The reviewer reads the diff
for details; the description should answer *what* and *why*, and call out any non-obvious
trade-off or deliberate decision.

```
**What does this PR do?**
Adds an EVP writer that batches flag evaluation events and ships them to the Agent every 10s
via the EVP proxy. The writer is fork-safe and drops events non-blocking on queue overflow.

**Motivation:**
Required by the FFE telemetry spec. Without this, flag evaluation counts never reach the backend.

**Change log entry**
Yes. Flag evaluation counts are now sent to Datadog via the EVP proxy.

**Additional Notes:**
Chose a canonical context key over MD5 so the encoding is auditable without a digest
dependency. Retry on transport failure is deferred — the writer logs and moves on for now.

**How to test the change?**
Covered by the new aggregator and writer specs; integration verified against mock intake.
```

Respond to every review comment with either a fix or an explanation of
why the change is not being made. Do not re-request review with open threads unresolved.

---

## Style

Run `bundle exec rake standard:fix` before pushing. StandardRB is fixed and non-configurable
by team convention.

---

**Tool note.** Claude Code reads `CLAUDE.md`, not `AGENTS.md`. If you use Claude Code,
symlink this file: `ln -s AGENTS.md CLAUDE.md` inside this directory. Verify how your
specific tool (Cursor, Copilot, Codex) discovers nested guide files.
Loading
Loading