Skip to content

Migrate Metrics Collector module to Logs Ingestion API - #7550

Open
Damon Barry (damonbarry) wants to merge 19 commits into
Azure:mainfrom
damonbarry:migrate-mc-api
Open

Migrate Metrics Collector module to Logs Ingestion API#7550
Damon Barry (damonbarry) wants to merge 19 commits into
Azure:mainfrom
damonbarry:migrate-mc-api

Conversation

@damonbarry

@damonbarry Damon Barry (damonbarry) commented Sep 9, 2026

Copy link
Copy Markdown
Member

Summary

The Azure Monitor HTTP Data Collector API — and the OMS agent certificate-registration/mTLS protocol this module used to talk to it — is retiring on 14 September 2026. This PR migrates the AzureMonitor upload target to the Logs Ingestion API, which is unaffected by the retirement. The IotMessage upload target is untouched.

Breaking changes

  • Config: LogAnalyticsWorkspaceId / LogAnalyticsSharedKey are removed. Replaced with DataCollectionEndpoint, DataCollectionRuleId, and DataCollectionStreamName, pointing at a Data Collection Endpoint (DCE) and Data Collection Rule (DCR).
  • Destination table: the Logs Ingestion API cannot write to the built-in InsightsMetrics table, so metrics now land in a customer-provisioned custom table (_CL suffix) reachable through the DCR. Field schema (Origin, Namespace, Name, Value, CollectionTime, Tags, Computer) is unchanged.
  • Auth model: shared-key auth is replaced with Microsoft Entra ID, via an explicit ChainedTokenCredential (certificate → client secret → workload identity federation → managed identity), configured through standard AZURE_* environment variables. See src/README.md for the full authentication section and per-scenario setup.

Implementation

  • Deleted CertificateGenerator/ and FixedSetTableUpload/AzureFixedSetTable.cs — the entire self-signed-cert/OMS-agent-registration/mTLS upload path is gone.
  • FixedSetTableUpload.PublishAsync now calls Azure.Monitor.Ingestion's LogsIngestionClient.UploadAsync (Azure.Monitor.Ingestion 1.2.0). The SDK handles batching/compression internally.
  • Added FixedSetTableUpload/AadCredentialFactory.cs to build the ChainedTokenCredential.
  • Robustness fixes found during manual validation against a live DCE/DCR:
    • Exceptions during an upload attempt are now caught per retry instead of aborting the whole retry loop on the first failure.
    • Metrics with NaN/Infinity values (legitimately emitted by summary/quantile metrics with no recent samples — see BuiltInMetrics.md) are filtered out before upload. System.Text.Json, which the SDK uses internally, throws on non-finite doubles by default; the SDK's own exception handling only accounts for OperationCanceledException and silently swallows anything else, which was surfacing as a confusing NullReferenceException inside LogsIngestionClient.UploadAsync on every scrape cycle. The old Newtonsoft-based path never hit this because Newtonsoft serializes NaN permissively.
  • Updated Settings.cs, Program.cs, Constants.cs, ExampleDeployment.json, packages.lock.json accordingly.
  • Updated dotnet.yaml's unit test env vars (LogAnalyticsWorkspaceId/SharedKeyDataCollectionEndpoint/DataCollectionRuleId/DataCollectionStreamName).
  • CHANGELOG.md and src/README.md updated with the breaking-change summary and new configuration/authentication docs.

Testing

  • All 28 existing unit tests pass; no test coverage existed previously for the upload/credential path (the old code had none either).
  • Manually validated end-to-end against a live Log Analytics workspace/DCE/DCR/IoT Hub with UploadTarget=AzureMonitor, including the managed-identity auth path and the NaN-metrics fix.

Follow-ups (not in this PR)

  • edge-util's AzureLogAnalytics.cs and the TestMetricsCollector/TestResultCoordinator E2E test modules still use the classic Data Collector API — also affected by the 14 Sept 2026 retirement, tracked separately.
  • The four curated IoT Edge Azure Monitor workbooks (in the external microsoft/Application-Insights-Workbooks repo) hardcode InsightsMetrics and will need updating to support a configurable custom table name.

Azure IoT Edge PR checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines and Best Practices

  • I have read the contribution guidelines.
  • Title of the pull request is clear and informative.
  • Description of the pull request includes a concise summary of the enhancement or bug fix.

Testing Guidelines

  • Pull request includes test coverage for the included changes.
  • Description of the pull request includes
    • concise summary of tests added/modified
    • local testing done.

@damonbarry
Damon Barry (damonbarry) marked this pull request as ready for review September 10, 2026 23:03
@damonbarry

Copy link
Copy Markdown
Member Author

Used agency copilot to do a panel review. The results:


Code Review — pr-7550-migrate-metrics-collector-module-to-logs-ingestion-api

REVIEW-CODE · Deep Review · 2026-09-10

A senior-engineer code review produced by REVIEW-CODE using competing models, cross-referenced and de-duplicated, plus a blind independent adversarial challenge.

Review Overview

Review Mode Deep Review
Scope Type Pull Request
Review Focus Full
Scope Boundary #7550, aggregate main...HEAD changes across the 15 files listed in Appendix A
Models Model A: Claude Opus 4.8 · Model B: GPT-6 Astra · Adversarial: Grok 4.6
Total Recommendations 9
Severity Breakdown Critical 0 · High 1 · Medium 6 · Low 2 · Informational 0
Not in Review Focus None — full review
Scope Test Coverage ~0% (manual direct-path estimate; denominator = newly added or materially changed upload, credential, and configuration branches)

Claude Opus 5 was selected initially for Model A but returned no usable output. The required single retry used Claude Opus 4.8.

Materials Provided

  • Associated Documents: Metrics Collector README and changelog; repository contribution guidance; Azure Monitor Data Collector and Logs Ingestion API documentation; IoT Edge metrics/workbook documentation.

Active Extensions & Source Legend

  • 🟦 Core — REVIEW-CODE core review

No extensions active — core review only.

Principal-Engineer Focus Areas

# Area / File:Line Why it warrants a closer look Related finding ID(s)
1 AadCredentialFactory.cs:19-38, FixedSetTableUpload.cs:31 Authority, token audience, and legacy configuration must remain aligned across public, US Government, and China clouds. 1.1
2 FixedSetTableUpload.cs:115-140, README.md:10-15 The replacement custom-table contract drops or leaves unspecified Azure Monitor metadata previously supplied by the classic endpoint. 1.2, 1.5
3 FixedSetTableUpload.cs:79-100 SDK-level batching plus an outer whole-request retry creates non-obvious partial-success and duplicate-ingestion semantics. 7.1

Test Coverage Summary

Coverage derivation: manual direct-path estimate. No in-scope test source changed, and the existing suite does not directly exercise the new or materially changed upload, credential, retry, authority-selection, or AzureMonitor settings branches.

Area (file:line / unit) What is untested (path / edge case / failure mode) Suggested test(s) to add
AadCredentialFactory.Create/GetAuthorityHost Public/Government/China authority and audience pairing; legacy China domain; user-assigned managed identity selection Table-driven cloud mapping tests and system-assigned/user-assigned identity construction tests
FixedSetTableUpload.PublishAsync Finite/non-finite filtering, empty batch, success/failure, partial batch failure, cancellation, retry termination Inject a mockable ingestion client and test mixed values, all-invalid values, partial failures, cancellation, and retry counts
Settings AzureMonitor branch Required DCE/DCR/stream values, HTTPS endpoint validation, and IotMessage independence Environment/configuration tests for valid and invalid AzureMonitor settings and an IotMessage configuration without DCR values
Custom-table payload/schema ResourceID, CollectionTimeTimeGenerated, and the seven documented metric fields Serialization/contract test against the expected DCR input schema
ExampleDeployment.json Image/configuration compatibility Manifest assertion that the image version supports the DCE/DCR settings present in the same file

Overall Assessment

The migration removes the retiring certificate/shared-key path cleanly and uses the supported ingestion SDK, but it is not ready for all environments represented by the existing configuration contract. Sovereign-cloud authentication is currently incomplete, and several data-contract and retry behaviors can produce total ingestion failure, lost resource/time semantics, or duplicate metrics. The absence of direct tests across the replacement path materially raises the risk of this deadline-driven breaking release.


Prioritized Recommendations (Critical → Minimal)

Start here: 1.1

ID Severity Section Source Origin Title Models
1.1 High Correctness 🟦 Core Original Configure sovereign-cloud authority and ingestion audience as one mapping Models A, B & Adversarial
1.2 Medium Correctness 🟦 Core ⚔️ Adversarial Preserve resource attribution in the custom-table contract Adversarial
1.3 Medium Correctness 🟦 Core Original Honor AZURE_CLIENT_ID for user-assigned managed identity Model B
1.4 Medium Correctness 🟦 Core Original Make the example image compatible with its new settings Model B
1.5 Medium Correctness 🟦 Core ⚔️ Adversarial Document or emit the required TimeGenerated mapping Adversarial
7.1 Medium Reliability & Resilience 🟦 Core Original Avoid whole-payload retries after partial SDK success Models A, B & Adversarial
8.1 Medium Test Coverage 🟦 Core Original Add direct regression coverage for the replacement path Models A, B & Adversarial
1.6 Low Correctness 🟦 Core Original Validate the DCE as an absolute HTTPS URI Model A & Adversarial
9.1 Low Observability 🟦 Core Original Preserve the terminal upload failure cause at an operational log level Model A

Detailed Findings

1. Correctness

1.1 Configure sovereign-cloud authority and ingestion audience as one mapping — High — 🟦 Core — Original — [Models A, B & Adversarial]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/AadCredentialFactory.cs:19-38; edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:31
  • Issue: The credential maps AzureDomain to a sovereign authority, but LogsIngestionClient is created without LogsIngestionClientOptions.Audience. Azure.Monitor.Ingestion 1.2.0 defaults to the public-cloud audience and explicitly requires an alternate audience for sovereign clouds. In addition, the new switch recognizes only azure.cn, while the pre-migration README documented azure.com.cn; that existing value now silently selects the public-cloud authority.
  • Why it matters: Government and China deployments can authenticate against the wrong authority or request a token for the wrong resource, causing every upload to fail after customers are forced off the retiring API.
  • Recommendation: Centralize AzureDomain mapping to return both AuthorityHost and the matching LogsIngestionAudience; preserve azure.com.cn as a supported value or provide an explicit compatibility alias; reject unknown non-public values; and test every supported cloud.

1.2 Preserve resource attribution in the custom-table contract — Medium — 🟦 Core — ⚔️ Adversarial — [Adversarial]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:115-140; edge-modules/metrics-collector/src/Settings.cs:39,160,184; edge-modules/metrics-collector/src/README.md:13,25-26
  • Issue: The old uploader sent ResourceID as x-ms-AzureResourceId, which supplied row-level resource attribution. The replacement payload contains only the seven LaMetric fields, while ResourceID remains mandatory, logged, and otherwise unused.
  • Why it matters: Resource-scoped queries, resource-context access, and workbook filtering can lose the IoT Hub association. Keeping the setting mandatory also tells operators that the value still affects AzureMonitor ingestion when it does not.
  • Recommendation: Carry ResourceId into the custom-table/DCR contract so the destination _ResourceId is populated, and document the mapping. If dropping attribution is intentional, remove the unused requirement and document the behavior as a breaking change.

1.3 Honor AZURE_CLIENT_ID for user-assigned managed identity — Medium — 🟦 Core — Original — [Model B]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/AadCredentialFactory.cs:21-26; edge-modules/metrics-collector/src/README.md:83-90
  • Issue: new ManagedIdentityCredential() selects the default/system-assigned identity. Unlike DefaultAzureCredentialOptions, this standalone constructor does not use AZURE_CLIENT_ID to select a user-assigned managed identity. The preceding credentials cannot use a lone managed-identity client ID, so the configured identity is ignored.
  • Why it matters: A host with both identity types can authenticate as the system identity even when the user-assigned identity named by AZURE_CLIENT_ID has the DCR role, producing authorization failures or using an unintended principal.
  • Recommendation: When AZURE_CLIENT_ID is present for managed identity, construct ManagedIdentityCredential with the corresponding ManagedIdentityId; otherwise retain system-assigned behavior. Document and test both cases.

1.4 Make the example image compatible with its new settings — Medium — 🟦 Core — Original — [Model B]

  • Location: edge-modules/metrics-collector/src/ExampleDeployment.json:35,43-50
  • Issue: The changed manifest supplies DCE/DCR/stream settings but still deploys mcr.microsoft.com/azureiotedge-metrics-collector:1.0.0, an image from before this configuration contract.
  • Why it matters: The README calls this a complete example; applying it deploys a module that expects workspace/shared-key settings and cannot consume the new values.
  • Recommendation: Update the example to a migration-compatible image version at the same time as the configuration keys.

1.5 Document or emit the required TimeGenerated mapping — Medium — 🟦 Core — ⚔️ Adversarial — [Adversarial]

  • Location: edge-modules/metrics-collector/src/README.md:10-15; edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:121,134
  • Issue: The setup instructions list CollectionTime in the incoming/custom schema but do not explain that every Azure Monitor Logs table requires TimeGenerated or show a DCR transform from CollectionTime. The payload itself emits only CollectionTime.
  • Why it matters: An operator following the setup literally can create a DCR that fails schema validation or records ingestion time instead of scrape time, producing incorrect time-series queries and alerts.
  • Recommendation: Emit TimeGenerated directly, or document a concrete DCR transformation such as source | extend TimeGenerated = todatetime(CollectionTime) and include the complete stream and destination schemas.

1.6 Validate the DCE as an absolute HTTPS URI — Low — 🟦 Core — Original — [Model A & Adversarial]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:26-31; edge-modules/metrics-collector/src/Settings.cs:41-45
  • Issue: DataCollectionEndpoint is checked only for whitespace before new Uri(...). A missing scheme throws outside the dedicated configuration-validation path; an http URI survives parsing and fails later when bearer-token transport is rejected. Program catches setup exceptions and returns success.
  • Why it matters: A common rollout typo becomes a generic setup error or repeated runtime failure rather than a clear, fail-fast configuration error.
  • Recommendation: Parse and validate an absolute https URI in Settings, throwing the same clear configuration exception used for other required AzureMonitor values.

2. Architecture & Design

No additional substantive findings beyond the testability seam described in 8.1.

3. Maintainability

No additional substantive findings.

4. Simplicity

No substantive findings.

5. Performance

No substantive findings.

6. Security

No exploitable security vulnerabilities were identified. Findings 1.1 and 1.3 concern authentication correctness and identity selection but fail closed under the reviewed configurations.

7. Reliability & Resilience

7.1 Avoid whole-payload retries after partial SDK success — Medium — 🟦 Core — Original — [Models A, B & Adversarial]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:79-100
  • Issue: The SDK partitions IEnumerable uploads into independently committed batches and throws AggregateException when a batch fails without an UploadFailed handler. The outer loop retries the complete list, so previously accepted batches can be ingested again. It also catches cancellation as an ordinary failure and immediately stacks three whole-operation attempts on top of Azure.Core's own retry policy.
  • Why it matters: Partial service failures can duplicate metrics, inflate ingestion cost, and distort aggregates. Shutdown cancellation produces misleading failures, while immediate nested retries amplify load without preserving idempotency.
  • Recommendation: Prefer the SDK's per-request retry policy. If application-level recovery is required, use LogsUploadOptions.UploadFailed to retain and retry only FailedLogs; propagate requested cancellation and avoid an immediate whole-payload retry loop.

8. Test Coverage

8.1 Add direct regression coverage for the replacement path — Medium — 🟦 Core — Original — [Models A, B & Adversarial]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/AadCredentialFactory.cs:17-39; edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:26-106; edge-modules/metrics-collector/src/Settings.cs:38-46
  • Issue: Scope Test Coverage is approximately 0% by manual direct-path estimate. No tests invoke the new credential chain, cloud mapping, SDK upload, non-finite filtering, retry/failure paths, or changed AzureMonitor configuration.
  • Why it matters: Every principal migration invariant—including the regression fix for NaN/infinity and all issues above—can regress without an automated signal.
  • Recommendation: Introduce an injectable SDK-client seam and add the concrete tests listed in the Test Coverage Summary, including mixed/all-nonfinite inputs, cloud and identity selection, partial failure, cancellation, retry count, schema serialization, endpoint validation, and IotMessage configuration independence.

9. Observability

9.1 Preserve the terminal upload failure cause at an operational log level — Low — 🟦 Core — Original — [Model A]

  • Location: edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs:86-105
  • Issue: Response status/reason and caught exceptions are logged only at Debug. After retries, the Error message contains only the metric count and retry count.
  • Why it matters: Common migration failures such as missing DCR role assignment, wrong rule/stream IDs, and schema mismatches are opaque at the default operational log level.
  • Recommendation: Retain the last response or exception and include its status/message in the terminal Error log, while avoiding sensitive payload or credential data.

10. Dependency Management

No substantive findings. The Azure Identity and Logs Ingestion dependencies are justified by the migration, their lockfile changes are consistent, and the obsolete certificate/compression dependencies are removed.


Model Cross-Reference Notes

  • Conflicts / disagreements and how they were resolved: None materially. Model A rated cancellation/backoff and endpoint validation lower than the adversarial pass; the report keeps endpoint validation Low and folds cancellation/retry amplification into the more concrete partial-batch duplication finding. The adversarial suggestions about a dead version constant and a stale README cross-reference were removed as low-value compared with the operational findings above.
  • Adversarial challenge: Grok 4.6 corroborated sovereign-cloud audience failure, whole-payload retry risk, and the test gap. It uniquely identified the dropped resource attribution and missing TimeGenerated mapping; both survived evidence checks and are marked ⚔️ Adversarial.

Accepted / Not Addressed

None — no findings were waived.


Appendix A — Review Context

  • Review Name: pr-7550-migrate-metrics-collector-module-to-logs-ingestion-api
  • Report File: code-reviews/pr-7550-migrate-metrics-collector-module-to-logs-ingestion-api-2026-09-10_1637.md
  • Date: 2026-09-10
  • Reviewer: REVIEW-CODE (senior software engineer)
  • Intake Confirmation: confirmed by user
  • Review Mode: Deep Review (default; includes adversarial pass)
  • Scope Type: Pull Request
  • Review Focus: Full
  • Scope Boundary: Migrate Metrics Collector module to Logs Ingestion API #7550, "Migrate Metrics Collector module to Logs Ingestion API," targeting main; the aggregate changes in the nine PR commits and the 15 files enumerated below.

Associated Documents

Document Location (path / URL / uploaded) Found Via Read Summary
Metrics Collector README edge-modules/metrics-collector/src/README.md innermost walk / changed file yes Defines deployment, DCE/DCR configuration, custom-table schema, Entra authentication chain, upload targets, and operational configuration.
Metrics Collector changelog edge-modules/metrics-collector/CHANGELOG.md changed file yes Records the 2.0.0 breaking migration from shared-key Data Collector API ingestion to Logs Ingestion API and Entra authentication.
Repository contribution guidance README.md (#contributing) PR link yes Directs contributors to doc/devguide.md and identifies the repository's principal components.
Azure Monitor HTTP Data Collector API https://learn.microsoft.com/previous-versions/azure/azure-monitor/logs/data-collector-api PR and work item links yes Documents the retiring shared-key HTTP ingestion API.
Logs Ingestion API overview https://learn.microsoft.com/azure/azure-monitor/logs/logs-ingestion-api-overview PR and work item links yes Describes DCR/DCE-based REST and client-library ingestion into Log Analytics.
Collect and transport Azure IoT Edge metrics https://learn.microsoft.com/azure/iot-edge/how-to-collect-and-transport-metrics?tabs=iothub work item link yes Documents configuring Metrics Collector and transporting built-in metrics to Azure Monitor.
Azure Monitor workbooks for IoT Edge https://learn.microsoft.com/azure/iot-edge/tutorial-monitor-with-workbooks work item link yes Documents monitoring IoT Edge deployment health and performance with workbooks.
Curated IoT Edge visualizations https://learn.microsoft.com/azure/iot-edge/how-to-explore-curated-visualizations?tabs=messaging work item link yes Documents curated workbook visualizations for IoT Edge built-in metrics.

Discovery root: For Metrics Collector source files, discovery stopped at the nearest project boundary (edge-modules/metrics-collector/src, which owns the project file and README). For builds/checkin/dotnet.yaml, the bounded walk reached the repository root without finding a nearer project document.

Assumptions

None — the assembled intake briefing was confirmed by the user.

PR Discussion Context

None. The pull request has no comments, inline threads, or submitted reviews.

Active Extensions

None (core review only). Reviewer alias resolved to damonb; no matching repository individual extensions or reviewer-scoped personal extensions were found.

Files In Scope

  • builds/checkin/dotnet.yaml
  • edge-modules/metrics-collector/CHANGELOG.md
  • edge-modules/metrics-collector/src/CertificateGenerator/CertGenerator.cs (deleted)
  • edge-modules/metrics-collector/src/Constants.cs
  • edge-modules/metrics-collector/src/ExampleDeployment.json
  • edge-modules/metrics-collector/src/FixedSetTableUpload/AadCredentialFactory.cs (added)
  • edge-modules/metrics-collector/src/FixedSetTableUpload/AzureFixedSetTable.cs (deleted)
  • edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.cs
  • edge-modules/metrics-collector/src/Microsoft.Azure.Devices.Edge.Azure.Monitor.csproj
  • edge-modules/metrics-collector/src/Program.cs
  • edge-modules/metrics-collector/src/README.md
  • edge-modules/metrics-collector/src/Settings.cs
  • edge-modules/metrics-collector/src/config/versionInfo.json
  • edge-modules/metrics-collector/src/packages.lock.json
  • edge-modules/metrics-collector/tests/Microsoft.Azure.Devices.Edge.Azure.Monitor.Test/packages.lock.json

Appendix B — Conventions & Context Notes

Codebase Conventions

  • Language / Toolchain: C# targeting .NET 10 through netcoreappVersion.props; SDK-style projects build in Debug, Release, and CheckInBuild configurations with warnings treated as errors. The repository uses dotnet build/dotnet test, and the check-in pipeline runs unit tests by category.
  • Naming: Types and public members use PascalCase; locals and private fields generally use camelCase. Async methods use the Async suffix, enforced as an .editorconfig naming suggestion. Existing Metrics Collector code contains some legacy deviations such as DNSName.
  • Formatting & Layout: Four-space indentation, UTF-8, final newline, and trimmed trailing whitespace are enforced by .editorconfig. Members are generally qualified with this.; namespace-scoped using directives and Microsoft copyright headers are common in production files.
  • Idioms & Patterns: Constructor and argument validation uses Microsoft.Azure.Devices.Edge.Util.Preconditions. Asynchronous I/O uses Task, cancellation tokens, and usually ConfigureAwait(false). The module separates scraping from publishing through IMetricsPublisher; periodic work is run through PeriodicTask. Existing code commonly catches operational exceptions at process/task boundaries, logs them, and returns a success flag where the publisher contract requires one.
  • Testing: xUnit 2.4 with Moq 4.18 under edge-modules/metrics-collector/tests/Microsoft.Azure.Devices.Edge.Azure.Monitor.Test. Tests use [Fact], Assert.*, direct construction, and descriptive Test... method names. The existing suite covers parsing, filtering, and metric value semantics, including NaN and infinities; the PR adds no new test source files.
  • Documentation: Operational configuration is maintained in src/README.md; release-impacting changes are recorded in CHANGELOG.md; deployment configuration is demonstrated in JSON manifests. Comments are used for non-obvious protocol, compatibility, and behavior rationale.
  • Observability: The module uses LoggerUtil.Writer with Microsoft.Extensions.Logging levels. Startup/configuration, upload success, exhausted retries, skipped metrics, and caught exceptions are logged. Metrics collection and publication run within a periodic task boundary.
  • Dependencies: Microsoft.Azure.Devices.Edge.Azure.Monitor.csproj directly references Azure.Identity 1.17.1, Azure.Monitor.Ingestion 1.2.0, Microsoft.Azure.Devices.Client, Microsoft.Extensions.Configuration, and Newtonsoft.Json. Package lock files are enabled globally and committed for production and tests. The PR removes the old SharpZipLib/BouncyCastle-based certificate path from this project and introduces Azure Monitor Ingestion SDK dependencies.
  • Security & Trust Boundaries: Environment variables provide DCE/DCR routing data and standard AZURE_* identity configuration. AadCredentialFactory constructs a production-only ChainedTokenCredential: environment credential, workload identity, then managed identity. Azure cloud selection derives from AzureDomain; developer CLI/IDE credentials are intentionally excluded. Scraped Prometheus metrics and tags are external input serialized for the Logs Ingestion API. Settings logging is expected not to expose credentials.
  • Architecture Seams: Program selects the IMetricsPublisher implementation based on UploadTarget. MetricsScrapeAndUpload owns filtering and orchestration; FixedSetTableUpload maps domain Metric objects to the Azure Monitor table schema and owns upload/retry behavior; Azure SDK credential construction is isolated in AadCredentialFactory. IotMessage publication is a separate implementation and is outside the behavioral migration.

Intent & Requirements

  • What the in-scope code must do: Replace Metrics Collector's retiring shared-key/certificate-registration Azure Monitor upload path with DCE/DCR-based Logs Ingestion API upload; authenticate with Microsoft Entra ID; preserve the emitted metric field schema (Origin, Namespace, Name, Value, CollectionTime, Tags, Computer); expose DCE URL, DCR immutable ID, and stream name configuration; retain the IotMessage path; document deployment and authentication; and update CI inputs and dependency locks.
  • Explicit Non-Goals / Out of Scope: The PR states that IotMessage upload behavior is unchanged. Remaining Data Collector API use in edge-util, TestMetricsCollector, and TestResultCoordinator, plus updates to curated workbooks in microsoft/Application-Insights-Workbooks, are follow-up work rather than part of this PR.
  • Known Edge Cases / Invariants: Prometheus permits NaN and infinite values, while standard JSON used by the ingestion SDK does not; such values are to be omitted from Azure Monitor uploads without blocking finite metrics. Upload failures are retried per attempt up to Constants.UploadMaxRetries. The destination custom table must retain the seven documented fields. Credential selection must not silently use developer-machine Azure CLI or IDE sessions. Sovereign cloud authority selection continues to derive from AzureDomain.
  • Stated Intent vs. Diff: The aggregate diff removes the certificate generator and classic fixed-table HTTP client; adds the Azure Identity and Logs Ingestion SDK path; replaces workspace/shared-key settings with DCE/DCR/stream settings across code, example deployment, CI, docs, and lockfiles; filters non-finite metrics; and updates the module version. This matches the PR's stated migration boundary. No test source changes accompany the upload and credential changes; the PR explicitly states those paths lacked prior automated coverage and records manual live validation.
  • Already Raised in PR Discussion: None.

Extension Focus (intimate-knowledge checks)

No active extensions — core review only.

@damonbarry

Copy link
Copy Markdown
Member Author

To address the findings of the panel review:


Implemented the recommended fixes in separate commits:

  • adf670d58 Configure Logs Ingestion for sovereign clouds

    • Adds matching authority and ingestion audience selection for public, Government, and China clouds.
    • Preserves the legacy azure.com.cn value.
    • Rejects unsupported Azure domains.
  • d9dc6fb27 Honor user-assigned managed identity configuration

    • Uses AZURE_CLIENT_ID to select a user-assigned managed identity.
    • Preserves system-assigned identity behavior when the variable is absent.
  • 705787d92 Preserve resource attribution in metrics uploads

    • Adds ResourceId to each uploaded metric.
    • Documents the DCR transform mapping ResourceId to _ResourceId.
    • Documents the CollectionTime to TimeGenerated transform as part of the new 2.0.0 schema.
  • 4111829ec Update metrics collector example image

    • Updates the example manifest from 1.0.0 to 2.0.0.
  • daa3efdad Validate the Logs Ingestion endpoint

    • Requires DataCollectionEndpoint to be an absolute HTTPS URI.
  • 46ffbff9a Log the terminal metrics upload failure

    • Includes the final exception or HTTP response details at Error level after retries are exhausted.
  • 7697a0e8a Add migration regression tests

    • Covers public, Government, China, and legacy China cloud mappings.
    • Covers unsupported Azure domains.
    • Covers mixed finite, NaN, and infinite metric filtering.

Validation completed:

  • Full Metrics Collector test suite: 34 passed
  • Release build: successful
  • Finding 7.1, concerning whole-payload retries after partial SDK success, remains intentionally deferred as previously recommended because it requires a larger retry/event-handler redesign.

@jlian

Copy link
Copy Markdown
Member

Docs and workbook updates in progress

Docs PR opened as draft https://github.com/MicrosoftDocs/azure-docs-pr/pull/320482

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.

2 participants