Skip to content

Use stable config for RUM snippet creation#331

Merged
pawelchcki merged 36 commits into
masterfrom
package_maybe_use_shared_library_for_env
May 6, 2026
Merged

Use stable config for RUM snippet creation#331
pawelchcki merged 36 commits into
masterfrom
package_maybe_use_shared_library_for_env

Conversation

@pawelchcki

@pawelchcki pawelchcki commented Mar 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Switches external configuration to stable config (file based config managed by the agent), which is used as baseline for RUM config.


Adopts inject-browser-sdk's new 3-arg snippet_create_from_stable_config(language, debug, json_overlay) API. The SDK now reads application_monitoring.yaml and merges an optional JSON overlay internally — on_datadog_rum_config builds the overlay from block directives; try_build_snippet_from_stable_config passes NULL.

inject-browser-sdk is vendored as a git submodule (deps/inject-browser-sdk). Build toolchain bumped to rust 1.94 to match the SDK's rust-toolchain.toml. Misleading test_env_* integration tests renamed to test_stable_config_* (they exercise YAML, not env vars).

- Auto path: try_build_snippet_from_env now uses
  snippet_create_from_stable_config to read from application_monitoring.yaml
  instead of building JSON from DD_RUM_* env vars
- Directive path: on_datadog_rum_config seeds from stable config via
  stable_config_get_entries, with directive block values overriding
- Trim env var forwarding to only DD_RUM_ENABLED and DD_RUM_APPLICATION_ID
- Pin inject-browser-sdk to 587b071 (includes stable_config_get_entries FFI)
A transitive dependency (clap_builder 4.6.0, via libdd-library-config)
now requires the Rust 2024 edition which needs Rust 1.85+.
@codecov-commenter

codecov-commenter commented Mar 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.85%. Comparing base (f19926a) to head (00b46ba).
⚠️ Report is 19 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #331   +/-   ##
=======================================
  Coverage   68.85%   68.85%           
=======================================
  Files          56       56           
  Lines        7471     7471           
  Branches     1058     1058           
=======================================
  Hits         5144     5144           
  Misses       1820     1820           
  Partials      507      507           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The :latest tag was cached by nydus snapshotter on CI runners.
Use an explicit tag to force pulling the updated image with Rust 1.85.0.
aws-lc-sys (transitive dep via libdd-library-config -> rustls) compiles
C code during its build script. Without sysroot flags, the linker
cannot find crtbeginS.o in the musl cross-compilation environment.
Rust_CARGO_TARGET is empty at the point corrosion_set_env_vars runs.
Use hardcoded x86_64/aarch64 target names instead — only one will
match the actual cargo build target, the other is harmlessly ignored.
aws-lc-sys build script compile+links test binaries via clang. Without
these flags, clang invokes system GNU ld which can't find crtbeginS.o
and libgcc in the musl sysroot. Using lld and compiler-rt matches the
rest of the toolchain setup.
test_env_only_config and test_partial_env_config now write
application_monitoring.yaml into the container instead of setting
DD_RUM_* env vars, matching the new stable config integration.
test_env_remote_configuration_id, test_env_disabled_overrides_env_config,
and test_env_disabled_location_override now use application_monitoring.yaml
instead of DD_RUM_* env vars. DD_RUM_ENABLED still uses env var since
it controls the enable/disable flag, not the config source.
…hared_library_for_env

# Conflicts:
#	.gitlab/build-and-test-fast.yml
#	.gitlab/common.yml
#	.gitlab/prepare-oci-package.sh
#	.gitlab/ssi-package.yml
Adds DD_RUM_REMOTE_CONFIGURATION_ID support in configuration_from_getter
so remoteConfigurationId appears in snippets created via stable config.
inject-browser-sdk gained a 3-arg snippet_create_from_stable_config
(language, debug_logs, json_overlay) that reads YAML and merges an
optional JSON overlay internally. Move both call sites to it:
- on_datadog_rum_config builds the overlay from block directives
  instead of pre-reading entries with the (now removed)
  stable_config_get_entries.
- try_build_snippet_from_env passes a NULL overlay.

Switch inject-browser-sdk back to a git submodule (deps/inject-browser-sdk),
add_subdirectory()d so its CMakeLists handles corrosion + cbindgen.
Bump build_env rust toolchain to 1.94.0 to match the SDK's rust-toolchain.

Add ngx_pcalloc stub so the catch2 unit_tests target links, and update
EXPECTED_BIG_CHONK to the SDK's new alphabetical key ordering.
These tests were originally written against DD_RUM_* environment
variables, then migrated to read from the stable-config YAML, but
their names and conf files were never updated. Rename for accuracy:

- test_env_only_config            -> test_stable_config_only
- test_partial_env_config         -> test_nginx_config_overlays_stable_config
- test_env_remote_configuration_id -> test_stable_config_remote_configuration_id
- test_env_disabled_overrides_env_config -> test_dd_rum_enabled_false_overrides_stable_config
- test_env_disabled_location_override -> test_location_off_overrides_stable_config
- rum_env_only.conf               -> rum_stable_config_only.conf
- rum_partial_env.conf            -> rum_stable_config_partial.conf
- <ENV_APP_ID> / <ENV_TOKEN> placeholders -> <STABLE_APP_ID> / <STABLE_TOKEN>
Comment thread src/rum/config.cpp Outdated
@pawelchcki
pawelchcki marked this pull request as ready for review April 30, 2026 14:57
@pawelchcki
pawelchcki requested a review from a team as a code owner April 30, 2026 14:57
Copilot AI review requested due to automatic review settings April 30, 2026 14:57
@pawelchcki
pawelchcki requested a review from a team as a code owner April 30, 2026 14:57

Copilot AI 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.

Pull request overview

This PR switches RUM snippet generation to prefer Datadog Agent “stable config” (application_monitoring.yaml) via snippet_create_from_stable_config, and updates build/test plumbing to vendor inject-browser-sdk as a submodule and adjust CI/toolchain versions.

Changes:

  • Update RUM config paths to generate snippets from stable config (with optional directive overlay) and reduce forwarded env vars.
  • Refactor tests to write/read stable config inside the nginx test container and add new nginx configs for stable-config scenarios.
  • Vendor inject-browser-sdk as a submodule and adjust build/CI images and nginx version matrices.

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/rum/config.cpp Switch snippet creation to stable-config API; adjust env var forwarding and directive overlay behavior.
test/cases/rum/test_injection.py Replace env-only RUM tests with stable-config-based tests using container-written YAML.
test/cases/rum/conf/rum_stable_config_only.conf New nginx config fixture for stable-config-only injection.
test/cases/rum/conf/rum_stable_config_partial.conf New nginx config fixture for “stable config + directive overrides” behavior.
test/unit/stub_nginx.c Add missing nginx allocator stub needed by updated config code paths.
deps/CMakeLists.txt Switch from FetchContent to submodule-based build of inject-browser-sdk; add cross-compilation env wiring.
.gitmodules Add deps/inject-browser-sdk submodule.
build_env/Dockerfile Update Rust toolchain version used in the musl toolchain image.
.gitlab/common.yml Pin build image tag to a Rust-specific toolchain image tag.
.gitlab/ssi-package.yml Pin SSI packaging job image tag to Rust-specific toolchain image tag.
.gitlab/build-and-test-fast.yml Pin shellcheck image tag and add nginx 1.29.5 to build/test matrices.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/rum/config.cpp
Comment thread .gitlab/common.yml Outdated
Comment thread build_env/Dockerfile Outdated
Comment thread build_env/Dockerfile Outdated
Comment thread src/rum/config.cpp Outdated
Comment thread src/rum/config.cpp Outdated
Comment thread src/rum/config.cpp
…hared_library_for_env

# Conflicts:
#	.gitlab/build-and-test-fast.yml
#	.gitlab/ssi-package.yml
- build_env/Dockerfile: replace Unicode en-dash in `–tlsv1.2` with
  ASCII `--tlsv1.2` so curl parses the option
- src/rum/config.cpp:
  - Rename try_build_snippet_from_env to try_build_snippet_from_stable_config
    (it no longer reads env vars; the stable-config YAML is the source)
  - Drop env-fallback telemetry tag block (DD_RUM_APPLICATION_ID lookup +
    hardcoded remote_config_used:false). Telemetry tags from the snippet
    path now leave fields empty when the nginx block doesn't supply them
    instead of misreporting `false`.
  - Drop hardcoded remote_config_used:false in apply_rum_config_tags so
    stable-config-supplied remoteConfigurationId isn't mis-tagged.
  - Remove DD_RUM_APPLICATION_ID from get_environment_variable_names —
    it's no longer read.
The merge of origin/master kept this branch's newer dd-trace-cpp ref
(f8c39137) over master's (92cdfe6). Roll back to master's pointer to
keep this PR scoped to RUM stable-config changes.
clang-format wraps the snippet_create_from_stable_config call onto one
line; yapf wraps Path(__file__) onto multiple lines so the conf paths
fit within the line length.
system-tests builds with RUM=OFF, so the private inject-browser-sdk
submodule is not needed. Init only dd-trace-cpp and libddwaf in CI to
avoid the auth failure on the private repo. Also normalize the
inject-browser-sdk URL to a relative path for consistency with the
other submodules.
@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Apr 30, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 67.62% (+0.00%)

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

- Remove get_rum_config_from_env, get_rum_enabled_from_env, rum_env_mappings
  and the env_mapping struct; only ever called from unit tests.
- Remove the corresponding catch2 tests and their ScopedEnv helpers.
- Factor the libdd-library-config "nginx" identifier into a rum_language
  constant in config_internal.h to remove the literal duplication.
- Trim verbose comment in deps/CMakeLists.txt and the WHAT comment on the
  overlay-build line in config.cpp; keep only the WHY about the SDK merge.
The github source is private and requires PAT/SSH auth that CI runners
don't have. The gitlab.ddbuild.io DataDog mirror is reachable via the
GitLab CI job token, so use it as the submodule URL.
parse_bool used an inline lambda over std::tolower; switch to the
existing datadog::nginx::to_lower helper from src/string_util.h.

Drop the WHAT comments on the two SDK-validation fallbacks in
make_rum_json_config — the if/else structure already conveys the
meaning ("not a number → pass as string").
Comment thread test/cases/rum/test_injection.py Outdated
Comment thread test/cases/rum/test_injection.py Outdated
Comment thread test/cases/rum/test_injection.py Outdated
Comment thread test/cases/rum/test_injection.py Outdated
Comment thread test/cases/rum/test_injection.py Outdated
Comment thread test/cases/rum/test_injection.py Outdated
pawelchcki added 8 commits May 6, 2026 00:11
Use yaml.safe_dump and a _stable_config context manager to dedupe
write/cleanup boilerplate across the five stable-config tests. Hoist
imports to the top, extract the config path constant, drop unused
captures, and add PyYAML to test deps.
Both _write_stable_config and _cleanup_stable_config had a single
caller (the new context manager); fold them in. Also use
Orchestration.nginx_replace_file for the write, which already handles
the mkdir -p $(dirname ...) heredoc pattern instead of duplicating it.
Mirrors nginx_replace_file so _stable_config no longer needs raw
subprocess/docker_compose_command/child_env plumbing for cleanup.
Restores the rum_remote_config_tag = "remote_config_used:false" default
that was lost in the env-var->stable-config migration, ensuring telemetry
never emits an empty tag value. Switches rum_language from string_view to
const char* since it is passed to a C FFI that requires null termination.
Also extracts a _read_conf test helper, refreshes stale "env vars"
comments, and restores the "exception" catch-parameter name for
codebase consistency.
- Extract add_member/add_first_as_string lambdas in make_rum_json_config
- Add make_stable_config_snippet helper to wrap Snippet unique_ptr
- Replace child_explicit/parent_explicit pair with single rum_enable_unset
- Drop redundant inline comment in reload_nginx_with_empty_config
The CI test image bakes /.venv from a snapshot of test/pyproject.toml,
refreshed only by a manual `make build-push-test-image`. New deps
(PyYAML for the RUM stable-config tests) didn't reach CI.

Switch to `uv run --project test test/bin/run.py ...` so each job
syncs test/.venv against the source pyproject.toml before running.
The coverage job inherits the same yaml-import failure: it uses the
build image (whose baked /.venv is also stale) and invokes run.py
directly. Switch the Makefile's test/test-openresty/coverage targets
to `uv run --project test` so they sync against the source pyproject.toml,
matching the pattern just applied to .gitlab/common.yml.
@pawelchcki
pawelchcki merged commit 205a709 into master May 6, 2026
178 checks passed
@pawelchcki
pawelchcki deleted the package_maybe_use_shared_library_for_env branch May 6, 2026 11:32
@pawelchcki

Copy link
Copy Markdown
Collaborator Author

Thanks @xavier-datadog

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.

4 participants