Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 21, 2025

This PR addresses the issue where Blazor ComponentsMetrics was adding "unknown" as tag values when the actual values were null, instead of omitting the tags entirely.

Problem

The current implementation in ComponentsMetrics.cs uses the pattern componentType ?? "unknown" when adding tags to telemetry measurements. This results in metrics containing tags with "unknown" values even when no meaningful value is available:

var tags = new TagList
{
    { "aspnetcore.components.type", componentType ?? "unknown" },
    { "code.function.name", methodName ?? "unknown" },
    { "aspnetcore.components.attribute.name", attributeName ?? "unknown" }
};

This behavior is inconsistent with other ASP.NET Core metrics implementations and telemetry best practices, where tags should only be present when they have meaningful values.

Solution

Following the established pattern used in Identity metrics (SignInManagerMetrics, UserManagerMetrics), this PR:

  1. Introduces helper methods that conditionally add tags only when values are not null:

    • AddComponentTypeTag
    • AddRouteTag
    • AddMethodNameTag
    • AddAttributeNameTag
    • AddErrorTag
  2. Updates all metric recording methods to use these helper methods instead of the ?? "unknown" pattern:

    • Navigation
    • CaptureEventDuration
    • FailEventSync
    • CaptureParametersDuration
    • FailParametersSync
    • CaptureBatchDuration
    • FailBatchSync
  3. Adds comprehensive tests to verify that tags are properly omitted when null values are passed.

Example

Before this change:

// Metrics would include tags like:
// aspnetcore.components.type = "unknown"
// code.function.name = "unknown"
componentsMetrics.CaptureEventDuration(task, timestamp, null, null, null);

After this change:

// Metrics will only include tags with actual values
// No "unknown" tags will be present
componentsMetrics.CaptureEventDuration(task, timestamp, null, null, null);

This change makes Blazor metrics consistent with the rest of the ASP.NET Core telemetry ecosystem and follows OpenTelemetry best practices for conditional tag inclusion.

Fixes #63350.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Don't add "unknown" to Blazor metrics Don't add "unknown" to Blazor metrics tags when values are null Aug 21, 2025
@Copilot Copilot AI requested a review from JamesNK August 21, 2025 01:29
Copilot finished work on behalf of JamesNK August 21, 2025 01:29
@JamesNK JamesNK marked this pull request as ready for review August 21, 2025 01:50
@Copilot Copilot AI review requested due to automatic review settings August 21, 2025 01:50
@JamesNK JamesNK requested a review from a team as a code owner August 21, 2025 01:50
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 removes the addition of "unknown" tag values in Blazor ComponentsMetrics when actual values are null, following telemetry best practices where tags should only be present when they have meaningful values.

  • Replaces inline tag creation with conditional helper methods that only add tags when values are not null
  • Updates all metric recording methods to use the new helper pattern instead of the ?? "unknown" fallback
  • Adds comprehensive test coverage to verify that null values result in omitted tags rather than "unknown" values

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/Components/Components/src/ComponentsMetrics.cs Refactors tag creation to use conditional helper methods instead of "unknown" fallbacks
src/Components/Components/test/ComponentsMetricsTest.cs Adds test coverage for null value scenarios to verify tags are properly omitted

@pavelsavara pavelsavara added the area-blazor Includes: Blazor, Razor Components label Aug 22, 2025
@pavelsavara
Copy link
Member

Microsoft.AspNetCore.Mvc.FunctionalTests.RazorBuildTest.RazorViews_AreUpdatedOnChange

Log

[6.180s] [Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker] [Verbose] Resource Filter: After executing OnResourceExecuted on filter Microsoft.AspNetCore.Mvc.ViewFeatures.Filters.SaveTempDataFilter.
[6.180s] [Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker] [Information] Executed action RazorBuildWebSite.UpdateableViewsController.Index (RazorBuildWebSite) in 638.2469ms
[6.180s] [Microsoft.AspNetCore.Routing.EndpointMiddleware] [Information] Executed endpoint 'RazorBuildWebSite.UpdateableViewsController.Index (RazorBuildWebSite)'
[6.180s] [Microsoft.AspNetCore.Hosting.Diagnostics] [Information] Request finished HTTP/1.1 GET http://localhost/UpdateableViews - 200 null text/html; charset=utf-8 1303.7462ms
[6.376s] [Microsoft.AspNetCore.Mvc.FunctionalTests.RazorBuildTest] [Error] Test threw an exception.
Xunit.Sdk.EqualException: Assert.Equal() Failure: Strings differ
           ↓ (pos 0)
Expected: "4to4d4m2.kc4, Version=0.0.0.0, Culture=ne"···
Actual:   "l43kb21p.cvy, Version=0.0.0.0, Culture=ne"···
           ↑ (pos 0)
   at Xunit.Assert.Equal(ReadOnlySpan`1 expected, ReadOnlySpan`1 actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences, Boolean ignoreAllWhiteSpace) in /_/src/xunit.assert/Asserts/StringAsserts.cs:line 965
   at Xunit.Assert.Equal(String expected, String actual, Boolean ignoreCase, Boolean ignoreLineEndingDifferences, Boolean ignoreWhiteSpaceDifferences, Boolean ignoreAllWhiteSpace) in /_/src/xunit.assert/Asserts/StringAsserts.cs:line 1342
   at Microsoft.AspNetCore.Mvc.FunctionalTests.RazorBuildTest.RazorViews_AreUpdatedOnChange() in /_/src/Mvc/test/Mvc.FunctionalTests/RazorBuildTest.cs:line 130
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass47_0.<<InvokeTestMethodAsync>b__1>d.MoveNext() in /_/src/xunit.execution/Sdk/Frameworks/Runners/TestInvoker.cs:line 259
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in /_/src/xunit.execution/Sdk/Frameworks/ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in /_/src/xunit.core/Sdk/ExceptionAggregator.cs:line 90
[6.377s] [Microsoft.Extensions.Hosting.Internal.Host] [Debug] Hosting stopping
[6.377s] [Microsoft.Hosting.Lifetime] [Information] Application is shutting down...
[6.377s] [Microsoft.Extensions.Hosting.Internal.Host] [Debug] Hosting stopped
[6.377s] [TestLifetime] [Information] Finished test RazorViews_AreUpdatedOnChange in 6.37568s

@ilonatommy
Copy link
Member

Microsoft.AspNetCore.Mvc.FunctionalTests.RazorBuildTest.RazorViews_AreUpdatedOnChange

This is not related, merge with main: #63358

@ilonatommy
Copy link
Member

@copilot, merge this branch with main.

Copy link
Contributor Author

Copilot AI commented Aug 22, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • ytvvsblobprodcus310.vsblob.vsassets.io
    • Triggering command: dotnet test ./src/Components/Components/test/Microsoft.AspNetCore.Components.Tests.csproj --filter ComponentsMetricsTest (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of ilonatommy August 22, 2025 11:10
@Copilot Copilot AI requested a review from ilonatommy August 22, 2025 11:10
@ilonatommy ilonatommy enabled auto-merge (squash) August 22, 2025 11:13
@ilonatommy ilonatommy merged commit 6a148ec into main Aug 22, 2025
29 checks passed
@ilonatommy ilonatommy deleted the copilot/fix-63350 branch August 22, 2025 13:07
@dotnet-policy-service dotnet-policy-service bot added this to the 11.0-preview1 milestone Aug 22, 2025
@pavelsavara
Copy link
Member

/backport to release/10.0-rc1

Copy link
Contributor

Started backporting to release/10.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/17156650495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Attention: Shared Code Modified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't add "unknown" to Blazor metrics
4 participants