Migrate Metrics Collector module to Logs Ingestion API - #7550
Migrate Metrics Collector module to Logs Ingestion API#7550Damon Barry (damonbarry) wants to merge 19 commits into
Conversation
|
Used Code Review — pr-7550-migrate-metrics-collector-module-to-logs-ingestion-apiREVIEW-CODE · Deep Review · 2026-09-10
Review Overview
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
Active Extensions & Source Legend
No extensions active — core review only. Principal-Engineer Focus Areas
Test Coverage SummaryCoverage 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.
Overall AssessmentThe 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:
Detailed Findings1. Correctness1.1 Configure sovereign-cloud authority and ingestion audience as one mapping — High — 🟦 Core — Original — [Models A, B & Adversarial]
1.2 Preserve resource attribution in the custom-table contract — Medium — 🟦 Core — ⚔️ Adversarial — [Adversarial]
1.3 Honor
|
| 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.yamledge-modules/metrics-collector/CHANGELOG.mdedge-modules/metrics-collector/src/CertificateGenerator/CertGenerator.cs(deleted)edge-modules/metrics-collector/src/Constants.csedge-modules/metrics-collector/src/ExampleDeployment.jsonedge-modules/metrics-collector/src/FixedSetTableUpload/AadCredentialFactory.cs(added)edge-modules/metrics-collector/src/FixedSetTableUpload/AzureFixedSetTable.cs(deleted)edge-modules/metrics-collector/src/FixedSetTableUpload/FixedSetTableUpload.csedge-modules/metrics-collector/src/Microsoft.Azure.Devices.Edge.Azure.Monitor.csprojedge-modules/metrics-collector/src/Program.csedge-modules/metrics-collector/src/README.mdedge-modules/metrics-collector/src/Settings.csedge-modules/metrics-collector/src/config/versionInfo.jsonedge-modules/metrics-collector/src/packages.lock.jsonedge-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 usesdotnet 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
Asyncsuffix, enforced as an.editorconfignaming suggestion. Existing Metrics Collector code contains some legacy deviations such asDNSName. - Formatting & Layout: Four-space indentation, UTF-8, final newline, and trimmed trailing whitespace are enforced by
.editorconfig. Members are generally qualified withthis.; namespace-scopedusingdirectives 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 usesTask, cancellation tokens, and usuallyConfigureAwait(false). The module separates scraping from publishing throughIMetricsPublisher; periodic work is run throughPeriodicTask. 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 descriptiveTest...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 inCHANGELOG.md; deployment configuration is demonstrated in JSON manifests. Comments are used for non-obvious protocol, compatibility, and behavior rationale. - Observability: The module uses
LoggerUtil.Writerwith 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.csprojdirectly 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.AadCredentialFactoryconstructs a production-onlyChainedTokenCredential: environment credential, workload identity, then managed identity. Azure cloud selection derives fromAzureDomain; 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:
Programselects theIMetricsPublisherimplementation based onUploadTarget.MetricsScrapeAndUploadowns filtering and orchestration;FixedSetTableUploadmaps domainMetricobjects to the Azure Monitor table schema and owns upload/retry behavior; Azure SDK credential construction is isolated inAadCredentialFactory.IotMessagepublication 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 theIotMessagepath; document deployment and authentication; and update CI inputs and dependency locks. - Explicit Non-Goals / Out of Scope: The PR states that
IotMessageupload behavior is unchanged. Remaining Data Collector API use inedge-util,TestMetricsCollector, andTestResultCoordinator, plus updates to curated workbooks inmicrosoft/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 fromAzureDomain. - 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.
|
To address the findings of the panel review: Implemented the recommended fixes in separate commits:
Validation completed:
|
|
Docs and workbook updates in progress Docs PR opened as draft https://github.com/MicrosoftDocs/azure-docs-pr/pull/320482 |
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
AzureMonitorupload target to the Logs Ingestion API, which is unaffected by the retirement. TheIotMessageupload target is untouched.Breaking changes
LogAnalyticsWorkspaceId/LogAnalyticsSharedKeyare removed. Replaced withDataCollectionEndpoint,DataCollectionRuleId, andDataCollectionStreamName, pointing at a Data Collection Endpoint (DCE) and Data Collection Rule (DCR).InsightsMetricstable, so metrics now land in a customer-provisioned custom table (_CLsuffix) reachable through the DCR. Field schema (Origin,Namespace,Name,Value,CollectionTime,Tags,Computer) is unchanged.ChainedTokenCredential(certificate → client secret → workload identity federation → managed identity), configured through standardAZURE_*environment variables. Seesrc/README.mdfor the full authentication section and per-scenario setup.Implementation
CertificateGenerator/andFixedSetTableUpload/AzureFixedSetTable.cs— the entire self-signed-cert/OMS-agent-registration/mTLS upload path is gone.FixedSetTableUpload.PublishAsyncnow callsAzure.Monitor.Ingestion'sLogsIngestionClient.UploadAsync(Azure.Monitor.Ingestion1.2.0). The SDK handles batching/compression internally.FixedSetTableUpload/AadCredentialFactory.csto build theChainedTokenCredential.NaN/Infinityvalues (legitimately emitted by summary/quantile metrics with no recent samples — seeBuiltInMetrics.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 forOperationCanceledExceptionand silently swallows anything else, which was surfacing as a confusingNullReferenceExceptioninsideLogsIngestionClient.UploadAsyncon every scrape cycle. The old Newtonsoft-based path never hit this because Newtonsoft serializesNaNpermissively.Settings.cs,Program.cs,Constants.cs,ExampleDeployment.json,packages.lock.jsonaccordingly.dotnet.yaml's unit test env vars (LogAnalyticsWorkspaceId/SharedKey→DataCollectionEndpoint/DataCollectionRuleId/DataCollectionStreamName).CHANGELOG.mdandsrc/README.mdupdated with the breaking-change summary and new configuration/authentication docs.Testing
UploadTarget=AzureMonitor, including the managed-identity auth path and theNaN-metrics fix.Follow-ups (not in this PR)
edge-util'sAzureLogAnalytics.csand theTestMetricsCollector/TestResultCoordinatorE2E test modules still use the classic Data Collector API — also affected by the 14 Sept 2026 retirement, tracked separately.microsoft/Application-Insights-Workbooksrepo) hardcodeInsightsMetricsand 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
Testing Guidelines