Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 31, 2025

Customer Impact

Race between initializing telemetry for a component can cause a dashboard error: #9559

The fixes in the PR:

  • Ensure telemetry is initialized at the start of OnInitializedAsync. This means it will always been initialized when parameters are updated.
  • Log a warning instead of throwing an error if not initialized.

Testing

Manual

Risk

Low

Regression?

Telemetry is new in 9.3. But it causes errors on existing pages that previously worked.

@Copilot Copilot AI review requested due to automatic review settings May 31, 2025 02:47
Copy link
Contributor

@Copilot 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 enhances the telemetry context by ensuring properties are only posted when initialized and by adding logging support. It also cleans up duplicate initialization calls and pins markdownlint to prevent unexpected new rules.

  • Extend UpdateTelemetryProperties and PostProperties to accept an ILogger and guard against uninitialized telemetry service
  • Update tests to pass a logger instance into telemetry property updates
  • Inject ILogger into components and remove redundant TelemetryContextProvider.Initialize calls

Reviewed Changes

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

Show a summary per file
File Description
src/Aspire.Dashboard/Components/Pages/ComponentTelemetryContext.cs Added ILogger parameter to UpdateTelemetryProperties and a null check in PostProperties
tests/Aspire.Dashboard.Tests/Telemetry/ComponentTelemetryContextTests.cs Updated tests to declare and pass a NullLogger instance
src/Aspire.Dashboard/Components/Pages/StructuredLogs.razor.cs Added TelemetryContextProvider.Initialize, updated telemetry calls to include Logger
src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Injected ILogger<Resources> and updated telemetry calls
src/Aspire.Dashboard/Components/Pages/Metrics.razor.cs Added TelemetryContextProvider.Initialize, updated telemetry calls to include Logger
src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor.cs Added TelemetryContextProvider.Initialize, updated telemetry calls to include Logger
src/Aspire.Dashboard/Components/Pages/Error.razor.cs Injected ILogger<Error>, reordered RequestId assignment, updated telemetry calls
src/Aspire.Dashboard/Components/Controls/ResourceDetails.razor.cs Injected ILogger<ResourceDetails>, updated telemetry calls
src/Aspire.Dashboard/Components/Pages/Traces.razor.cs Moved and de-duplicated TelemetryContextProvider.Initialize
src/Aspire.Dashboard/Components/Pages/TraceDetail.razor.cs Moved and de-duplicated TelemetryContextProvider.Initialize
src/Aspire.Dashboard/Components/Pages/Login.razor.cs Moved and de-duplicated TelemetryContextProvider.Initialize
Comments suppressed due to low confidence (3)

src/Aspire.Dashboard/Components/Pages/StructuredLogs.razor.cs:149

  • The component calls UpdateTelemetryProperties(..., Logger) but does not inject a Logger. Add an [Inject] public required ILogger<StructuredLogs> Logger { get; init; } property to this component.
protected override void OnInitialized()

src/Aspire.Dashboard/Components/Pages/Metrics.razor.cs:78

  • The component calls UpdateTelemetryProperties(..., Logger) but does not inject a Logger. Add an [Inject] public required ILogger<Metrics> Logger { get; init; } property to this component.
protected override void OnInitialized()

src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor.cs:128

  • The component calls UpdateTelemetryProperties(..., Logger) but does not inject a Logger. Add an [Inject] public required ILogger<ConsoleLogs> Logger { get; init; } property to this component.
protected override async Task OnInitializedAsync()

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Jun 2, 2025
@danmoseley danmoseley requested a review from adamint June 2, 2025 12:12
@danmoseley
Copy link
Member

there was a merge conflict resolved to put this in 9.3 -- have you or @adamint verified the fix functions in its 9.3 form? if so, good to go

@adamint
Copy link
Member

adamint commented Jun 2, 2025

@danmoseley yep, verified after pulling down branch

@danmoseley danmoseley merged commit b50d00e into release/9.3 Jun 2, 2025
177 checks passed
@danmoseley danmoseley deleted the jamesnk/telemetry-9.3 branch June 2, 2025 14:14
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-dashboard Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants