Skip to content

Respect child-scope override for tracing directives#62

Merged
pawelchcki merged 3 commits intomainfrom
pc/tri-state-merge
Apr 22, 2026
Merged

Respect child-scope override for tracing directives#62
pawelchcki merged 3 commits intomainfrom
pc/tri-state-merge

Conversation

@pawelchcki
Copy link
Copy Markdown
Contributor

@pawelchcki pawelchcki commented Apr 21, 2026 •
edited
Loading

tracing_enabled and trust_inbound_span were plain bools defaulting to true, and merge_dir_conf OR'd parent with child — so DatadogTracing Off (or DatadogTrustInboundSpan Off) inside a <Location> was silently dropped: the enclosing scope's implicit true always won the OR.

Switch both Directory fields to std::optional<bool> so an explicit directive (true/false) is distinguishable from "unset" (nullopt). Merge now falls back to the parent only when the child never set the directive; read sites apply the default via .value_or(true).

Follow-up: #65 tracks adding an integration test for DatadogTracing Off in a child scope (the DatadogTrustInboundSpan case is already covered by test_trust_inbound_span_directive).

@pawelchcki pawelchcki mentioned this pull request Apr 21, 2026
Open
@pawelchcki Graphite App
Copy link
Copy Markdown
Contributor Author

pawelchcki commented Apr 21, 2026 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Apr 21, 2026
Merged
Open
Open
@pawelchcki pawelchcki marked this pull request as ready for review April 21, 2026 13:56
@pawelchcki pawelchcki requested a review from a team as a code owner April 21, 2026 13:56
@pawelchcki pawelchcki requested review from Copilot and tabgok and removed request for a team and Copilot April 21, 2026 13:56
Copilot AI review requested due to automatic review settings April 21, 2026 14:40
Copilot AI reviewed Apr 21, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

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 fixes Apache directory-scope configuration merging so child scopes can explicitly override tracing directives (e.g., DatadogTracing Off inside a <Location>), instead of being forced on by an enclosing implicit default.

Changes:

  • Switched Directory::tracing_enabled and Directory::trust_inbound_span from bool to std::optional<bool> to distinguish “unset/inherit” vs “explicitly set”.
  • Updated per-directory merge logic to inherit only when the child scope didn’t set a value.
  • Updated tracing hook read sites to apply the effective default (true) via .value_or(true).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
mod_datadog/src/tracing/hooks.cpp Applies default-on behavior at read sites using value_or(true) for the new optional config fields.
mod_datadog/src/common_conf.h Converts the per-directory flags to std::optional<bool> and documents inheritance semantics.
mod_datadog/src/common_conf.cpp Implements tri-state merge so explicit child directives override parent/default settings.

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

Comment thread mod_datadog/src/tracing/hooks.cpp Outdated
Comment thread mod_datadog/src/common_conf.cpp
Comment thread mod_datadog/src/common_conf.cpp
@pawelchcki pawelchcki changed the base branch from pc/submodule-deps to graphite-base/62 April 21, 2026 17:10
@pawelchcki pawelchcki changed the base branch from graphite-base/62 to pc/submodule-deps April 21, 2026 17:33
@datadog-datadog-prod-us1

This comment has been minimized.

Sign in to view
@pawelchcki pawelchcki force-pushed the pc/tri-state-merge branch 2 times, most recently from a8401e5 to 8aecbac Compare April 21, 2026 18:27
@pawelchcki pawelchcki changed the base branch from pc/submodule-deps to graphite-base/62 April 21, 2026 18:42
@pawelchcki
Respect child-scope override for tracing directives
066310c 
tracing_enabled and trust_inbound_span were plain `bool` with a default
of `true`, and merge_dir_conf OR'd parent and child together. That
silently dropped `DatadogTracing Off` inside a <Location>: the
enclosing scope's implicit `true` always won the OR, so tracing stayed
on for the child.

Switch both Directory fields to std::optional<bool> so the directive
handler's explicit write (true/false) is distinguishable from the
default (nullopt). Merge now falls back to the parent only when the
child never set the directive; read sites apply the default via
.value_or(true).
@pawelchcki pawelchcki changed the base branch from graphite-base/62 to main April 21, 2026 18:43
@pawelchcki
Fully qualify datadog::conf::Directory cast in subrequest path
f8e8f97 
Matches the already-qualified cast on the non-subrequest branch a few
lines below. Namespace lookup currently happens to resolve, but would
break if another `conf` enters visibility in this TU.
@pawelchcki pawelchcki mentioned this pull request Apr 21, 2026
Open
xlamorlette-datadog
xlamorlette-datadog approved these changes Apr 22, 2026
View reviewed changes
Comment thread mod_datadog/src/common_conf.cpp
Comment thread mod_datadog/src/common_conf.cpp
Comment thread mod_datadog/src/tracing/hooks.cpp
ap_get_module_config(main_r->per_dir_config, datadog_module));
if (dir_conf == nullptr || !dir_conf->tracing_enabled) return DECLINED;
if (dir_conf == nullptr || !dir_conf->tracing_enabled.value_or(true))
return DECLINED;
Copy link
Copy Markdown
Contributor

@xlamorlette-datadog xlamorlette-datadog Apr 22, 2026

Choose a reason for hiding this comment

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

Please add braces when if … return is not on a single line (to avoid potential bug when adding a line before the return).
Same below, line 121.

@pawelchcki pawelchcki merged commit e0fab02 into main Apr 22, 2026
4 checks passed
@pawelchcki pawelchcki deleted the pc/tri-state-merge branch April 22, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@xlamorlette-datadog xlamorlette-datadog xlamorlette-datadog approved these changes

@tabgok tabgok Awaiting requested review from tabgok tabgok is a code owner automatically assigned from DataDog/dd-trace-cpp

@zacharycmontoya zacharycmontoya Awaiting requested review from zacharycmontoya

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pawelchcki @xlamorlette-datadog