Skip to content

[Dynamic Control] Phase 3b: Add PolicyStore - #5017

Open
stevejgordon wants to merge 6 commits into
open-telemetry:mainfrom
stevejgordon:dynamic-control-phase-3-store
Open

[Dynamic Control] Phase 3b: Add PolicyStore#5017
stevejgordon wants to merge 6 commits into
open-telemetry:mainfrom
stevejgordon:dynamic-control-phase-3-store

Conversation

@stevejgordon

Copy link
Copy Markdown
Contributor

Contributes to: #4742

Changes

Continues the Dynamic Control component implementation with the policy store layer.

  • Replaces the ITelemetryPolicy interface with an abstract TelemetryPolicy base class, enabling shared behaviour and a cleaner extension point for concrete immutable policy types.
  • Reorders PolicySourceKind to match the provider-priority order defined in the Telemetry Policy OTEP.
  • Adds PolicySourceVersion for sequence-based versioning of per-source policy data.
  • Adds an internal copy-on-write PolicyStore built on immutable snapshots with:
    • Stale-write rejection via sequence numbers
    • Version-based suppression to skip no-op updates
    • Metadata pinning (source registration metadata captured at write time)
    • Deterministic policy and source ordering via PolicyKeyComparer
    • Atomic snapshot replacement for lock-free reads

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions Bot added infra Infra work - CI/CD, code coverage, linters dependencies Pull requests that update a dependency file labels Aug 20, 2026
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.79518% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.09%. Comparing base (43d997b) to head (a10b164).
⚠️ Report is 9 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...amicControl/Internal/Policies/PolicyKeyComparer.cs 66.66% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5017      +/-   ##
==========================================
- Coverage   78.11%   78.09%   -0.02%     
==========================================
  Files         477      484       +7     
  Lines       20395    20552     +157     
==========================================
+ Hits        15931    16050     +119     
- Misses       4464     4502      +38     
Flag Coverage Δ
unittests-Contrib.Shared.Tests 88.49% <ø> (ø)
unittests-DynamicControl 99.14% <98.79%> (-0.86%) ⬇️
unittests-Exporter.Geneva 56.66% <ø> (-0.38%) ⬇️
unittests-Exporter.InfluxDB 97.96% <ø> (ø)
unittests-Exporter.OneCollector 95.60% <ø> (ø)
unittests-Extensions 95.77% <ø> (ø)
unittests-Extensions.Enrichment 100.00% <ø> (ø)
unittests-Extensions.Enrichment.AspNetCore 86.27% <ø> (ø)
unittests-Extensions.Enrichment.Http 94.33% <ø> (ø)
unittests-Instrumentation.AWS 85.35% <ø> (ø)
unittests-Instrumentation.AspNet 79.15% <ø> (ø)
unittests-Instrumentation.AspNetCore 87.20% <ø> (ø)
unittests-Instrumentation.Cassandra 92.30% <ø> (ø)
unittests-Instrumentation.ConfluentKafka 83.42% <ø> (ø)
unittests-Instrumentation.ElasticsearchClient 80.36% <ø> (ø)
unittests-Instrumentation.EntityFrameworkCore 81.02% <ø> (ø)
unittests-Instrumentation.EventCounters 76.31% <ø> (ø)
unittests-Instrumentation.GrpcCore 91.12% <ø> (ø)
unittests-Instrumentation.GrpcNetClient 78.30% <ø> (ø)
unittests-Instrumentation.Hangfire 88.88% <ø> (ø)
unittests-Instrumentation.Http 76.00% <ø> (ø)
unittests-Instrumentation.Kusto 92.44% <ø> (ø)
unittests-Instrumentation.Owin 89.15% <ø> (ø)
unittests-Instrumentation.Process 100.00% <ø> (ø)
unittests-Instrumentation.Quartz 77.77% <ø> (ø)
unittests-Instrumentation.Remoting 65.59% <ø> (ø)
unittests-Instrumentation.Runtime 100.00% <ø> (ø)
unittests-Instrumentation.ServiceFabricRemoting 39.91% <ø> (ø)
unittests-Instrumentation.SqlClient 84.88% <ø> (ø)
unittests-Instrumentation.StackExchangeRedis 94.98% <ø> (ø)
unittests-Instrumentation.Wcf 82.64% <ø> (ø)
unittests-OpAmp.Client 86.78% <ø> (-0.45%) ⬇️
unittests-PersistentStorage 67.74% <ø> (-1.47%) ⬇️
unittests-PowerShellScripts 86.27% <ø> (ø)
unittests-Resources.AWS 72.78% <ø> (ø)
unittests-Resources.Azure 84.18% <ø> (-3.39%) ⬇️
unittests-Resources.Container 80.76% <ø> (ø)
unittests-Resources.Gcp 80.70% <ø> (ø)
unittests-Resources.Host 72.34% <ø> (ø)
unittests-Resources.OperatingSystem 77.16% <ø> (ø)
unittests-Resources.Process 90.90% <ø> (ø)
unittests-Resources.ProcessRuntime 80.76% <ø> (ø)
unittests-Sampler.AWS 96.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...etry.DynamicControl/Internal/Policies/PolicyKey.cs 100.00% <100.00%> (ø)
...ynamicControl/Internal/Policies/TelemetryPolicy.cs 100.00% <100.00%> (ø)
...ntrol/Internal/Policies/TraceSamplingRatePolicy.cs 100.00% <100.00%> (ø)
...icControl/Internal/Sources/PolicySourceMetadata.cs 100.00% <100.00%> (ø)
...micControl/Internal/Sources/PolicySourceVersion.cs 100.00% <100.00%> (ø)
...icControl/Internal/Sources/SourceRegistrationId.cs 100.00% <ø> (ø)
...amicControl/Internal/Store/PolicySourceSnapshot.cs 100.00% <100.00%> (ø)
...metry.DynamicControl/Internal/Store/PolicyStore.cs 100.00% <100.00%> (ø)
...namicControl/Internal/Store/PolicyStoreSnapshot.cs 100.00% <100.00%> (ø)
...cControl/Internal/Store/PolicyStoreUpdateResult.cs 100.00% <100.00%> (ø)
... and 1 more

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stevejgordon
stevejgordon marked this pull request as ready for review August 20, 2026 15:10
@stevejgordon
stevejgordon requested a review from a team as a code owner August 20, 2026 15:10
@stevejgordon

Copy link
Copy Markdown
Contributor Author

Regarding the codecov. The missing line is technically unreachable but required to satisfy CS8509. Three options:

  1. Ignore it (but codecov % can slip over time if we have more similar scenarios that we add.
  2. Use ExcludeFromCodeCoverage on the method, but any future arms could be missed from testing without codecov detection.
  3. Use a reflection-based test which is a slight test hack but would cover the actual code. Only issue is it's tied to the method name etc.

Do you have a preference @martincostello?

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 20, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-21 15:41 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@martincostello

martincostello commented Aug 20, 2026

Copy link
Copy Markdown
Member

It's fine to not have 100% coverage, particularly for stuff that can't be reasonably covered or is just to hit a number.

It's just for new code, particularly when you're using an agent, I figured aiming for higher coverage is reasonable. In the previous PR I think there was a number of non-trivial paths that didn't have coverage even though it was still 90%+.

Also even though it's an ❌, it's just to draw attention to it - it's not a hard requirement we enforce.

Comment thread src/OpenTelemetry.DynamicControl/CHANGELOG.md Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Policies/PolicyKeyComparer.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Policies/PolicyKeyComparer.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Sources/PolicySourceMetadata.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Sources/PolicySourceMetadata.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicyStore.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicyStore.cs
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicyStore.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicyStore.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicyStoreSnapshot.cs Outdated
Comment thread src/OpenTelemetry.DynamicControl/Internal/Store/PolicySourceSnapshot.cs Outdated
{
snapshot = null;
error = "The metadata must not be a default PolicySourceMetadata instance. " +
"A default value bypasses constructor validation and would assign this snapshot top precedence.";

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.

That's not a security boundary though, is it? I would have thought it enough to tell the caller they're "holding it wrong" and not have to explain why (the internal design reasons).

@stevejgordon

Copy link
Copy Markdown
Contributor Author

That's not a security boundary though, is it?

It's a correctness boundary, default has no registration identity and its default priority value 0 would place the snapshot at the top of the priority ordering, silently corrupting the store. I'll look to rephrase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file infra Infra work - CI/CD, code coverage, linters

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants