Skip to content

feat(processors): configurable root fields via ECSCTX_ROOT_FIELDS#14

Merged
jab3z merged 1 commit into
mainfrom
feat/configurable-root-fields
Jun 11, 2026
Merged

feat(processors): configurable root fields via ECSCTX_ROOT_FIELDS#14
jab3z merged 1 commit into
mainfrom
feat/configurable-root-fields

Conversation

@jab3z

@jab3z jab3z commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What

Lets a consuming service promote additional log keys to root level (instead of extra.*) without ecsctx hardcoding the service's domain schema.

Came out of the deltabulk log verification for ottuco/core_backend#145: customer.* lands under extra.customer because ROOT_ALLOWLIST is a hardcoded frozenset. That placement is fine for now, but the consuming service should own that decision.

How

Mirrors the proven ECSCTX_MASK_EXEMPT_PATHS three-level pattern exactly (precedence: explicit call > Django setting > env var):

  • configure_root_fields(extra_fields=[...]) / configure_root_fields_from_env() in processors.py; reshape_log_event resolves ROOT_ALLOWLIST | custom lazily
  • Django bridge _auto_configure_root_fields() reads the ECSCTX_ROOT_FIELDS setting at log time from contextvars_injector (not in setup_logging, per 97e42cf)
  • ECSCTX_ROOT_FIELDS env var (CSV) as framework-agnostic fallback
  • ROOT_ALLOWLIST is only ever extended, never reduced

Tests

  • TestRootFieldsConfig (core: default, configured, env, explicit-beats-env)
  • TestRootFieldsSettingsBridge (Django: setting via auto-configure + via first processor call, absent-setting noop, explicit-beats-setting, settings-not-ready retry)
  • 150 passed; ruff clean on touched files

Release

No version bump in this PR — dispatch the Release workflow with patch after merge → v0.5.5 on PyPI. ottuco/core_backend will then bump to ecsctx[django]>=0.5.5.

A consuming service can promote additional keys to root (instead of
extra.*) without ecsctx hardcoding its domain schema. Mirrors the
masking-exemption pattern: configure_root_fields() > Django
ECSCTX_ROOT_FIELDS setting (lazy bridge at log time) > env var (CSV).
ROOT_ALLOWLIST is only ever extended, never reduced.
@jab3z

jab3z commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown

Claude finished @jab3z's task in 2m 35s —— View job


Code Review

  • Gather context and read changed files
  • Review core processor changes (ecsctx/processors.py)
  • Review Django bridge changes (ecsctx/contrib/django/processors.py)
  • Review public API changes (ecsctx/__init__.py)
  • Review tests (tests/test_processors.py, tests/test_django_processors.py, tests/conftest.py)
  • Review README documentation
  • Compile and post final review

Overall Assessment

Clean, correct implementation. The three-level precedence pattern mirrors ECSCTX_MASK_EXEMPT_PATHS exactly — consistent behavior, predictable semantics, and the test coverage is thorough. No bugs found. A few observations below, ranging from a real edge case to style/minor.


Findings

1. Ordering hazard: reshape_log_event called before first Django log silently ignores the setting

_get_root_allowlist() calls configure_root_fields_from_env() lazily:

# ecsctx/processors.py:129
def _get_root_allowlist() -> frozenset:
    if _custom_root_fields is None:
        configure_root_fields_from_env()
    return ROOT_ALLOWLIST | (_custom_root_fields or frozenset())

If reshape_log_event is called directly before the first Django log (before contextvars_injector_auto_configure_root_fields()), configure_root_fields_from_env() runs, sets _custom_root_fields = frozenset() (empty), and root_fields_are_configured() becomes True. The Django bridge then early-exits, silently ignoring ECSCTX_ROOT_FIELDS in Django settings.

This has the same shape as _get_exempt_patterns() / _auto_configure_masking(), so it's a known accepted trade-off in this codebase. Just worth documenting that callers shouldn't invoke reshape_log_event directly before the pipeline has had a chance to run contextvars_injector. Low severity in practice since reshape_log_event is only called through namespace_ecs_fields, which is downstream of contextvars_injector in the pipeline.

2. configure_root_fields_from_env() hits os.environ.get() on every log call until configured

# ecsctx/processors.py:128-131
def _get_root_allowlist() -> frozenset:
    if _custom_root_fields is None:
        configure_root_fields_from_env()
    ...

Before any configuration runs, every call to reshape_log_event triggers an env lookup. Same as _get_exempt_patterns() — consistent, but worth knowing in high-throughput paths. No action needed unless profiling surfaces it.

3. _reset_root_fields_settings_flag is not wired into the global autouse fixture

conftest.py autouse resets _reset_root_fields() (core state), but _root_fields_settings_attempted in django/processors.py is only reset by TestRootFieldsSettingsBridge's own class-level fixture. Consistent with how _reset_masking_settings_flag is handled. But if a future test outside TestRootFieldsSettingsBridge calls contextvars_injector and then checks the bridge behavior, the stale flag could be surprising.

Suggest adding _reset_root_fields_settings_flag to conftest.py alongside _reset_masking_settings_flag (if/when that fixture is added there too). Not a current bug.

4. No type guard on configure_root_fields(extra_fields=...)

# ecsctx/processors.py:109
_custom_root_fields = frozenset(f for f in (extra_fields or []) if f)

Non-string items pass the if f guard (e.g., [123, "customer"]). An integer key will never match a string log-event key, so it's silently a no-op rather than an error. The masking API has the same pattern. Not a real bug, but a isinstance(f, str) guard would make misconfiguration more obvious. Low priority.

5. root_fields_are_configured() semantics after an env-load with no env var

After _get_root_allowlist() triggers configure_root_fields_from_env() with no env var set, _custom_root_fields = frozenset() and root_fields_are_configured() returns True — even though nothing was actually configured. This is exactly how masking_is_configured() works, so it's internally consistent. Just worth keeping in mind if the predicate is used for diagnostics (it means "configure attempt was made," not "non-empty fields are active").


Test Coverage

Comprehensive and well-structured:

  • Default/configured/env/explicit-beats-env in core
  • Django: setting via direct call, via processor first-call, absent-setting noop, explicit-beats-setting, settings-not-ready retry
  • Autouse _reset_root_fields_module ensures isolation between tests

One gap worth considering: ECSCTX_ROOT_FIELDS="" (explicitly empty env var) — though the logic correctly produces frozenset() and falls back to ROOT_ALLOWLIST, an explicit test would document the contract. Minor.


Documentation

README additions are accurate and the three-code-block example (Django / env var / programmatic) matches the implementation. The env-var table row is placed correctly.


Summary

No blocking issues. The implementation is correct and consistent with the codebase's established patterns. The findings above are informational — the most actionable one is considering whether to guard _reset_root_fields_settings_flag in a global conftest fixture for future test hygiene. Everything else is an accepted trade-off already present in the masking config.

@jab3z jab3z merged commit 8c4024e into main Jun 11, 2026
3 checks passed
@jab3z jab3z deleted the feat/configurable-root-fields branch June 11, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant