Repository navigation
Conversation
Route every routed persistence path through one budgeted provider, so the process-wide cap is no longer enforced per partition. Evict globally oldest-first across every directory under the partition root rather than only partitions opened in this process, and evict only when the budget is what is in the way and the candidates cover the shortfall.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
AAD token exposure, drain and storage-budget races, and gate-off payload changes must be resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds opt-in multi-tenant trace routing, endpoint-partitioned persistence, and endpoint-scoped redirect caching.
Changes:
- Routes traces by instrumentation key and ingestion endpoint.
- Adds shared-budget tenant storage and shutdown draining.
- Adds extensive routing, persistence, and redirect tests.
File summaries
| File | Description |
|---|---|
PersistOnShutdownTests.cs |
Updates concurrent-drain assertions. |
MultiTenantStorageTests.cs |
Tests partitioning and storage budgets. |
MultiTenantStorageIntegrationTests.cs |
Tests routed persistence and replay. |
MultiTenantRoutingTests.cs |
Tests validation, grouping, and gating. |
MultiTenantIntegrationTests.cs |
Tests end-to-end routing and redirects. |
CommonTestFramework/MockTransmitter.cs |
Supports routed-send test capture. |
TransmitFromStorageHandler.cs |
Adds endpoint-specific draining. |
TraceHelper.cs |
Converts traces into endpoint groups. |
SemanticSlotMap.cs |
Registers routing attributes. |
SemanticSlot.cs |
Adds routing slots. |
SemanticConventions.cs |
Defines routing attribute names. |
MultiTenant/TenantRouting.cs |
Validates and normalizes endpoints. |
MultiTenant/MultiTenantStorage.cs |
Implements partitioned storage. |
MultiTenant/MultiTenantConfig.cs |
Defines the feature switch. |
MultiTenant/IMultiTenantTransmitter.cs |
Defines routed transmission. |
MultiTenant/EndpointRouteBatch.cs |
Pools endpoint groups. |
MultiTenant/BudgetedBlobProvider.cs |
Enforces shared storage budgeting. |
IngestionRedirectPolicy.cs |
Keys redirects by origin endpoint. |
HttpPipelineHelper.cs |
Suppresses routed customer statistics. |
Diagnostics/AzureMonitorExporterEventSource.cs |
Adds feature diagnostics. |
AzureMonitorTransmitter.cs |
Sends and persists routed groups. |
ExporterRegistrationHostedService.cs |
Disables Live Metrics when enabled. |
Customizations/ApplicationInsightsRestClient.cs |
Supports per-request endpoints. |
AzureMonitorTraceExporter.cs |
Selects the routed export path. |
CHANGELOG.md |
Documents the feature and redirect fix. |
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Routes each Activity to one of several Application Insights components by stamping the routing attributes, so the feature can be exercised against real regional ingestion endpoints. Connection strings come from MULTITENANT_HOST_CONNECTION_STRING and MULTITENANT_ROUTE_CONNECTION_STRINGS. The optional 'down' argument answers 503 for the tenant stamps without going to the network, which exercises per-endpoint back-off, persistence, and the drain on recovery.
… writes Rate-limited sampling counts traces per process, so one limit would be divided between tenants by arrival order alone. Multi-tenant now uses the fixed-rate sampler, which applies the same proportion to every tenant. Stop emitting the _APPRESOURCEPREVIEW_ resource metric on the routed path. It describes the host process, so filing it under a tenant's instrumentation key reported the host's identity as that tenant's own application. Report a refused routed write from BudgetedBlobProvider, which every persistence path shares, and log the endpoint each storage partition belongs to.
Refuse multi-tenant export when Entra credentials are configured. The bearer token policy sits in the shared pipeline, so a token scoped to the exporter's own audience would be attached to requests addressed to hosts named by telemetry. Claim the routing slots only on the routed path. Registering them globally took those attributes out of custom dimensions with nothing to emit them instead, so single-tenant telemetry lost them silently. Reserve budget bytes with a compare-and-swap before writing, so concurrent writers cannot each observe the same room and exceed the shared cap. Use the drain completion source as the drain slot, closing the window where a caller could wait on a no-op task while the real upload was still running. Name the endpoint in the back-off event, delete the sibling partition root in test cleanup, and drop a duplicated changelog link.
Restore the gate-off request cost: the redirect origin key is materialized only once something can use it, so a pipeline that has never been redirected pays neither a Uri, a string, nor a lock per request. Expired entries now free their slot instead of pinning the cache full for the life of the process. Wait on an in-flight drain rather than recomposing when there are no tenant partitions, so shutdown cannot start a second pass against a budget it has already spent and dispose the pipeline underneath it. Apply the storage recount as a correction rather than an assignment, so it cannot discard reservations made while it was scanning, and serialize eviction so two writers cannot credit the same deleted file twice. Check disposal before the partition fast path and empty the dictionary before tearing partitions down. Dispose the state manager before refusing an Entra configuration, report Live Metrics suppression once per process rather than per signal, attribute routed persistence exceptions to the routed path, and correct four stale comments.
Guard IdnHost in RedirectPolicyHelper. A routed endpoint is named by telemetry and can answer with a malformed punycode Location, which threw out of the pipeline before any response was seen: the endpoint was never backed off, its backlog grew every export, and the shared budget then evicted other tenants. Return the winning drain's task to a caller that lost the race, instead of one that completes immediately while the upload is still running. Stop holding the drain lock across the wait, which Dispose also takes. Compute the eviction shortfall under the eviction lock and perform the write outside it, and sample the recount baseline after the walk so a concurrent write is not counted twice. Make the disposal flag volatile, assert the transmitter actually ran, and assert the refused Entra construction disposed what it had built.
The routed partitions each report the endpoint they backed off; the host's own state manager was constructed without one, so its event carried an empty field.
Route names are derived from the endpoint host, so two tenants in the same region collided. Names are now disambiguated, and the run reports how many distinct endpoints the routes reduce to, which is the number of POSTs to expect.
Cover the routed path with a real resource, which characterizes the host cloud role a routed envelope carries so that changing it becomes deliberate. Prove routed exports report no customer SDK stats, with a single-tenant export as the control: without it the assertion held even against a mock transmitter that never reaches the code emitting them. Cover tenants that share an ingestion endpoint, both in memory and through storage: one partition, one blob carrying both keys, and one replay returning both. Add a concurrent writer test for the shared budget.
The mock keyed responses by host and answered 200 for any path, so a request that landed somewhere the API is not still looked delivered. It now keys by the whole endpoint and answers 404 for a path no stamp serves, which strengthens every test in the file. That makes the reason the redirect cache key includes the path testable: two tenants behind one gateway host, distinguished only by path, must not inherit each other's redirect. Verified by keying on authority alone, which fails it.
A redirect target that answered with an error was cached for the full cache lifetime (12h by default). Nothing invalidated it, because the replayed request is no longer a redirect, so the loop that set it never ran again. Only cache a target that answered. Eviction reserved once and then deleted, so a writer could evict blobs that a concurrent drain had already removed. Recount when the total is more than a second stale, retry the reservation before entering the eviction lock, and re-check it before each delete. Tests: assert the drain added a request in the replay tests, assert routed sends happened in the customer-stats test, prove a failed redirect target is not reused, drop a duplicated test, and correct two names and two comments that overstated what they covered.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
The branch was 42 commits behind main, which moved OpenTelemetry.PersistentStorage.FileSystem from 1.0.3 to 1.1.1. In 1.1.1 the byte[] TryCreateBlob overloads are obsolete in favour of ReadOnlySpan<byte>, and the repo builds obsolete usage as an error, so CI failed to compile while a local build against 1.0.3 still passed. Matches how main updated PersistentStorageExtensions in the same bump.
There was a problem hiding this comment.
🟡 Changes recommended
Shared-storage accounting can exceed its budget or destructively evict telemetry, and unbounded sequential endpoint sends can stall exports.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/MultiTenant/MultiTenantStorage.cs:187
- Eviction is committed before the destination write is known to succeed. When the budget is full and
inner.TryCreateBlobthen fails (for example because the partition directory became unwritable or the disk is full), older telemetry from other tenants has already been deleted and this method still returns failure. This contradicts the stated guarantee that non-budget write failures should not destroy backlog; the write/eviction flow needs a staging or rollback-safe design.
// Outside the lock: the write is the slow part, and holding it here would serialize
// every partition's failure path behind one disk write.
return reserved && TryCreateBlob(inner, buffer, leasePeriodMilliseconds, out blob);
- Files reviewed: 34/34 changed files
- Comments generated: 2
- Review effort level: Balanced
Harsimar Kaur (harsimar)
left a comment
There was a problem hiding this comment.
some non-blocking thoughts:
- a customer could theoretically specify a non-application insights endpoint in the attribute. I suppose the same is true for connection strings supplied via code or env var, but I'm assuming that customer app is within the trust boundary so this isn't as large of a concern.
- do we want to enforce a limit on the number of tenants?
Mostly configured by managed service running within trusted boundary where proxy-based URL is not supported.
Earlier I had some limits on max number of connections to a tenant. Both enforcing number of tenants and max connection limit could be a follow up after stable release. |
PR #62707 (multi-tenant trace export) merged 2026-09-09, after the 1.9.0 exporter release dated 2026-09-04, so its changelog entry was misplaced under the already-released 1.9.0 section. Move it up to the unreleased 1.10.0-beta.1 Features Added section alongside the logs entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
* Add multi-tenant export support for logs
Extend the trace multi-tenant routing mechanism to AzureMonitorLogExporter /
LogRecord: route logs to the correct ingestion endpoint based on the
microsoft.instrumentation_key / microsoft.ingestion_endpoint attributes,
reusing the existing signal-agnostic transmitter/storage/redirect plumbing.
Off by default behind the same EnableMultiTenantExport switch.
- TenantRouting: add raw-string TryGetRoute overload sharing validation.
- LogsHelper: add TryGetLogRoute, BuildLogTelemetryItem, and
OtelToAzureMonitorLogsMultiTenant; consume routing tags via
recognizeRoutingTags in ProcessLogRecordProperties/ExtractAvailabilityInfo.
- AzureMonitorLogExporter: multi-tenant gate + pooled EndpointRouteBatch export.
- Generalize event 61 message ("Telemetry is routed") and add CHANGELOG entry.
- Tests: MultiTenantLogRoutingTests (routing, parity, availability ordering,
exporter gate, trace-based filtering, persist-on-shutdown Track path).
- Demo: MultiTenantLogDemo wired into Program.cs via "multitenant logs".
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
* Clarify unrouted-record comments on the multi-tenant log path
Reword the "no routing attributes" comments to explain the mechanism
(routing attributes are stamped upstream only on records meant to be
routed) instead of asserting a deployment-specific assumption about how
many tenants enable observability. Comment-only; no behavior change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
* Emit all log telemetry types in the multi-tenant log demo
GenerateLogs now cycles through the four shapes the log exporter can
produce - trace (MessageData), exception (ExceptionData), custom event
(EventData), and availability (AvailabilityData) - by attaching the
classifying microsoft.* attributes through a list-valued log state.
Enable IncludeFormattedMessage so event/availability bodies survive.
Verified end to end: routed, persisted envelopes carry ExceptionData,
EventData, MessageData, and AvailabilityData under the tenant iKeys.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
* Move multi-tenant trace changelog entry to unreleased 1.10.0-beta.1
PR #62707 (multi-tenant trace export) merged 2026-09-09, after the
1.9.0 exporter release dated 2026-09-04, so its changelog entry was
misplaced under the already-released 1.9.0 section. Move it up to the
unreleased 1.10.0-beta.1 Features Added section alongside the logs entry.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dacfc0c4-4e41-460d-a866-cffed26f9bff
…#62823) * Add benchmarks for multi-tenant export and the shared redirect policy Multi-tenant trace export (#62707) shipped with no benchmark coverage. Two of the three questions it left open concern code that is dormant behind the AppContext switch; the third concerns the shared HTTP pipeline and is therefore live for every single-tenant caller. IngestionRedirectPolicyBenchmarks measures the per-request cost of the policy with and without a learned redirect. A learned redirect doubles the policy cost and adds 248 B, but that is 460 ns against an ingestion POST measured in milliseconds, so the correctness fixes cost the single-tenant path nothing that matters. MultiTenantExportBenchmarks compares the routed conversion with the single-tenant one and scales the endpoint count over 1, 3 and 25. Routing is not more expensive per Activity. Grouping is flat to three endpoints and rises by roughly half at 25, which is the ordinal linear scan in GetOrAdd behaving as documented; it remains under a tenth of conversion cost, and allocation stays flat, so the design holds at the supported scale. The single-tenant benchmark is deliberately independent of endpoint count so its rows act as a noise floor for the comparison. Measured results and their interpretation are recorded in each file header. * Correct two invalid measurements found in review GroupOnly formatted its endpoint strings inside the measured region, so it was largely measuring string.Format: all 72 KB of its reported allocation was the benchmark's own garbage, and the shape of the result was an artifact. With the strings built once in setup and indexed, grouping is 998 ns, 2.5 us and 16.2 us at 1, 3 and 25 endpoints with zero allocation - linear in endpoint count rather than flat, and now actual evidence that the pooling holds. Indexing also matches production, where NormalizeEndpoint memoizes and the ordinal comparison short-circuits on reference equality. The single-tenant baseline carried the routing attributes, which it serializes as two custom dimensions and the routed path does not, so the comparison was biased in favour of routing. The header explained the gap by a resource envelope that monitorBaseData null guarantees was never built. Both baselines are now measured, with and without the routing attributes, and the corrected numbers reverse the earlier conclusion: routing is not cheaper, it is within noise at one to three endpoints and about 19 percent slower at 25. The redirect warm-up asserted nothing, so a silently unwarmed cache would have turned two rows into duplicates of the baseline; it now probes and throws. Cost attribution corrected: the other-origin row pays the key and lock for 173 ns, so the remaining 386 ns is the trust check and rewrite, not the lock. * Keep the stub 200 header-free and correct the benchmark interpretation The previous commit moved Cache-Control onto the 200 so the policy would actually read it, but the 200 answers every benchmark request, so a header was built inside the measured region and added 224 B to every transport-using row. It is now attached only to the 200 that completes a redirect. With it gone the redirect table tightens sharply: StdDev falls from 55-171 ns to 7-14 ns, and the learned-redirect cost resolves to 373 ns split evenly between the cache key plus lock and the trust check plus rewrite, rather than the 173/386 split the noisier run suggested. Two interpretation errors corrected. The allocation gap was attributed to the routed path not serializing the routing attributes, but both baselines allocate 766,208 B whether they carry those attributes or not, so that cannot be the cause; the 8,448 B is the per-call List<TelemetryItem> growth and the schema counter that only single-tenant allocates. Percentages were computed against per-row baselines the header itself declares to be a 12% noise source, which manufactured both the 4% floor and the 39% ceiling; baselines are now pooled. Also removed a claim that ToLowerInvariant allocates, which it does not when no character changes. Grouping is described as flat cost per comparison against an average scan depth of (N+1)/2 rather than linear in endpoint count, which over-predicted 25 endpoints.
Adds multi-tenant trace export to the Azure Monitor exporter, off by default.
What it does
When the
Azure.Monitor.OpenTelemetry.EnableMultiTenantExportAppContext switch is set, the traceexporter routes each
Activityby themicrosoft.instrumentation_keyandmicrosoft.ingestion_endpointattributes it carries, instead of using the exporter's own connectionstring. Activities are grouped by ingestion endpoint and one request is issued per group.
An Activity missing either attribute, or carrying an endpoint that fails validation, is dropped. It
is never sent using another tenant's configuration. Most Activities reaching the exporter are
expected to be dropped, because only a fraction of tenants enable observability, so a batch that
drops entirely is reported as success rather than as an export failure.
Live Metrics is disabled while the switch is on.
Shape of the change
Internals/MultiTenant/holds everything new: the gate, endpoint validation and normalization,the pooled per-export grouping structure, the per-endpoint storage partitions, and the budgeted
blob provider.
AzureMonitorTraceExporter.Exportbranches to the routed path only when the gate is on.<storageDir>.tenantsroot, with one100 MB budget shared across every partition. Eviction is globally oldest-first, which complements
the drain's existing newest-first send order.
Also fixed here
The ingestion redirect cache was not keyed by the endpoint that issued the redirect, so a redirect
from one endpoint could be applied to a request bound for another. Separately, a redirect target was
cached even when the request to it failed, and nothing could invalidate that entry because the
replayed request is no longer a redirect, so a bad
307wedged an endpoint for the full cachelifetime. Neither is reachable on
main, where one pipeline serves one endpoint, but both becomereachable as soon as a pipeline serves several.
Review notes
Gate-off is not byte-for-byte identical to
main. The routed path adds nothing when the switchis off, but the two redirect fixes above do change the shared pipeline:
CreateRequestnow builds afresh URI builder per request rather than reusing one (that reuse is what let a redirect permanently
retarget every later request), and once any redirect has been learned the policy materializes a key
and takes a lock per request. Before the first redirect it does neither. Both are required by the
fixes.
Routed telemetry still carries the host's
ai.cloud.roleandroleInstance. Routing changes thedestination and the instrumentation key; it does not yet give the envelope the routed tenant's
identity. Deliberately left open: the right source for that identity is a design question I would
like input on.
Drain deletions do not decrement the shared storage byte counter. The drain reads blobs through a
pass-through, so deletes bypass the accounting until the next recount. Eviction now recounts when the
total is more than a second stale, which bounds the drift; the fuller fix is to make the wrapper
decrement on delete.
Testing
1021 tests pass on
net8.0andnet10.0. Clean build onnetstandard2.0,net8.0andnet10.0.Verified end to end against live Application Insights: four tenants across three ingestion endpoints
produced three requests, with the two tenants sharing a region delivered in one batch and split
correctly into their own components. An induced stamp outage persisted to the expected partitions and
replayed on recovery.
Not covered
Logs and metrics remain single-tenant. Standard metrics stay host-scoped. AAD is out of scope, and
multi-tenant refuses to start when a credential is configured.
net462is not supported or verified.The AOT compatibility app has not been run and no benchmark scenario was added.