Skip to content

Conversation

@adamint
Copy link
Member

@adamint adamint commented May 1, 2025

Description

Removes unnecessary properties that are also sensitive data, such as trace ids. Makes a few requested changes from the previous PR. After discussion with privacy folk, we should avoid sending custom command and resource type names; recommended approach is to only post names for known resource types/commands. For types, I generated a list based on implementations of Resource throughout the repo, filtering out any test resource.

re: #8925

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@adamint adamint requested a review from JamesNK May 1, 2025 10:24
@danmoseley danmoseley added this to the 9.3 milestone May 1, 2025
new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsFilterCount, new AspireTelemetryProperty(ViewModel.Filters.Count.ToString(CultureInfo.InvariantCulture), AspireTelemetryPropertyType.Metric)),
new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsTraceId, new AspireTelemetryProperty(TraceId ?? string.Empty)),
new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsSpanId, new AspireTelemetryProperty(SpanId ?? string.Empty))
new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsFilterCount, new AspireTelemetryProperty(ViewModel.Filters.Count.ToString(CultureInfo.InvariantCulture), AspireTelemetryPropertyType.Metric))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed you're doing a ToString here but not for TelemetryPropertyKeys.MetricsInstrumentsCount. Why the difference?

public const string Container = "Container";

// This field needs to be updated when new resource types are added.
private static readonly ImmutableArray<string> s_builtInResources = ["Resource", "AzureAppConfigurationResource", "AzureContainerAppEnvironmentResource", "AzureApplicationInsightsResource", "AzureOpenAIDeploymentResource", "AzureOpenAIResource", "AzureCosmosDBContainerResource", "AzureCosmosDBDatabaseResource", "AzureCosmosDBEmulatorResource", "AzureCosmosDBResource", "AzureEventHubConsumerGroupResource", "AzureEventHubResource", "AzureEventHubsEmulatorResource", "AzureEventHubsResource", "AzureFunctionsProjectResource", "AzureKeyVaultResource", "AzureLogAnalyticsWorkspaceResource", "AzurePostgresFlexibleServerDatabaseResource", "AzurePostgresFlexibleServerResource", "AzurePostgresResource", "AzureRedisCacheResource", "AzureRedisResource", "AzureSearchResource", "AzureServiceBusEmulatorResource", "AzureServiceBusQueueResource", "AzureServiceBusResource", "AzureServiceBusSubscriptionResource", "AzureServiceBusTopicResource", "AzureSignalREmulatorResource", "AzureSignalRResource", "AzureSqlDatabaseResource", "AzureSqlServerResource", "AzureBlobStorageResource", "AzureQueueStorageResource", "AzureStorageEmulatorResource", "AzureStorageResource", "AzureTableStorageResource", "AzureWebPubSubHubResource", "AzureWebPubSubResource", "AppIdentityResource", "AzureBicepResource", "AzureProvisioningResource", "DockerComposeEnvironmentResource", "DockerComposeServiceResource", "ElasticsearchResource", "GarnetResource", "KafkaServerResource", "KafkaUIContainerResource", "KeycloakResource", "KubernetesEnvironmentResource", "KubernetesResource", "AttuResource", "MilvusDatabaseResource", "MilvusServerResource", "MongoDBDatabaseResource", "MongoDBServerResource", "MongoExpressContainerResource", "MySqlDatabaseResource", "MySqlServerResource", "PhpMyAdminContainerResource", "NatsServerResource", "NodeAppResource", "OracleDatabaseResource", "OracleDatabaseServerResource", "PgAdminContainerResource", "PgWebContainerResource", "PostgresDatabaseResource", "PostgresServerResource", "PythonAppResource", "PythonProjectResource", "QdrantServerResource", "RabbitMQServerResource", "RedisCommanderResource", "RedisInsightResource", "RedisResource", "SeqResource", "SqlServerDatabaseResource", "SqlServerServerResource", "ValkeyResource", "ContainerResource", "ExecutableResource", "ParameterResource", "ProjectResource", "ConnectionStringParameterResource", "ConnectionStringResource", "ExecutableContainerResource", "ProjectContainerResource"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list doesn't seem to have executable, project or container. Do they have resource in the end when tested? e.g. ExecutableResource. Can you double check and maybe add a test. Add a comment if they're not needed explaining why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list had ExecutableResource, ProjectResource, and ContainerResource. These seem to be the only special cases where the resource type differs from the class name. I've updated them

adamint added 3 commits May 2, 2025 10:06
…rce suffix from executable/project/container. ToString MetricsInstrumentsCount
# Conflicts:
#	src/Aspire.Dashboard/Components/Pages/Metrics.razor.cs
new ComponentTelemetryProperty(TelemetryPropertyKeys.MetricsInstrumentsCount, new AspireTelemetryProperty(PageViewModel.Instruments?.Count ?? -1)),
new ComponentTelemetryProperty(TelemetryPropertyKeys.MetricsSelectedMeter, new AspireTelemetryProperty(PageViewModel.SelectedMeter?.Name ?? string.Empty)),
new ComponentTelemetryProperty(TelemetryPropertyKeys.MetricsSelectedInstrument, new AspireTelemetryProperty(PageViewModel.SelectedInstrument?.Name ?? string.Empty)),
new ComponentTelemetryProperty(TelemetryPropertyKeys.MetricsInstrumentsCount, new AspireTelemetryProperty((PageViewModel.Instruments?.Count ?? -1).ToString(CultureInfo.InvariantCulture), AspireTelemetryPropertyType.Metric)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So string is prefered? I want to know because I have telemetry that has a numeric value

{
new(TelemetryPropertyKeys.ResourceView, new AspireTelemetryProperty(PageViewModel.SelectedViewKind.ToString(), AspireTelemetryPropertyType.UserSetting))
new(TelemetryPropertyKeys.ResourceView, new AspireTelemetryProperty(PageViewModel.SelectedViewKind.ToString(), AspireTelemetryPropertyType.UserSetting)),
new(TelemetryPropertyKeys.ResourceTypes, new AspireTelemetryProperty(_resourceByName.Values.Select(r => TelemetryPropertyValues.GetResourceTypeTelemetryValue(r.ResourceType)).OrderBy(t => t).ToList()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're no longer passing a count of the resource types and just having a list of strings. Why this change? I'm asking because I have telemetry that passed in multiple values with counts like before this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the prior approach is that each of the keys count as a different telemetry property and each will need to be classified. That's a manual process and will need to be done for every resource type.

Copy link
Member

@JamesNK JamesNK May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a distinct here? The current output will contain duplicates, e.g. 50 containers means container is repeated 50 times. That doesn't seem desirable.

Or is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional so that we can count the number of occurrences of different types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional so that we can count the number of occurrences of different types.

@adamint adamint merged commit 7a53a08 into dotnet:main May 5, 2025
170 checks passed
Comment on lines +12 to +13
// This field needs to be updated when new resource types are added.
private static readonly HashSet<string> s_builtInResources = ["AppIdentityResource", "AttuResource", "AzureAppConfigurationResource", "AzureApplicationInsightsResource", "AzureBicepResource", "AzureBlobStorageResource", "AzureContainerAppEnvironmentResource", "AzureCosmosDBContainerResource", "AzureCosmosDBDatabaseResource", "AzureCosmosDBEmulatorResource", "AzureCosmosDBResource", "AzureEventHubConsumerGroupResource", "AzureEventHubResource", "AzureEventHubsEmulatorResource", "AzureEventHubsResource", "AzureFunctionsProjectResource", "AzureKeyVaultResource", "AzureLogAnalyticsWorkspaceResource", "AzureOpenAIDeploymentResource", "AzureOpenAIResource", "AzurePostgresFlexibleServerDatabaseResource", "AzurePostgresFlexibleServerResource", "AzurePostgresResource", "AzureProvisioningResource", "AzureQueueStorageResource", "AzureRedisCacheResource", "AzureRedisResource", "AzureSearchResource", "AzureServiceBusEmulatorResource", "AzureServiceBusQueueResource", "AzureServiceBusResource", "AzureServiceBusSubscriptionResource", "AzureServiceBusTopicResource", "AzureSignalREmulatorResource", "AzureSignalRResource", "AzureSqlDatabaseResource", "AzureSqlServerResource", "AzureStorageEmulatorResource", "AzureStorageResource", "AzureTableStorageResource", "AzureWebPubSubHubResource", "AzureWebPubSubResource", "ConnectionStringParameterResource", "ConnectionStringResource", Container, "DockerComposeEnvironmentResource", "DockerComposeServiceResource", "ElasticsearchResource", Executable, "ExecutableContainerResource", "GarnetResource", "KafkaServerResource", "KafkaUIContainerResource", "KeycloakResource", "KubernetesEnvironmentResource", "KubernetesResource", "MilvusDatabaseResource", "MilvusServerResource", "MongoDBDatabaseResource", "MongoDBServerResource", "MongoExpressContainerResource", "MySqlDatabaseResource", "MySqlServerResource", "NatsServerResource", "NodeAppResource", "OracleDatabaseResource", "OracleDatabaseServerResource", "ParameterResource", "PgAdminContainerResource", "PgWebContainerResource", "PhpMyAdminContainerResource", "PostgresDatabaseResource", "PostgresServerResource", Project, "ProjectContainerResource", "PythonAppResource", "PythonProjectResource", "QdrantServerResource", "RabbitMQServerResource", "RedisCommanderResource", "RedisInsightResource", "RedisResource", "Resource", "SeqResource", "SqlServerDatabaseResource", "SqlServerServerResource", "ValkeyResource"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. We shouldn't be treating Azure resources any different than AWS resources. Similarly, CommunityToolkit resources shouldn't act differently than resources we define in our repo.

Is there a better/different way to accomplish what we are trying to do here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to maintain an allow list of resources

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is going to be an acceptable solution. This list is going to get out of date, and already has. Maybe a better way is to add an annotation on our Resources, AllowTelemetryOptInAnnotation or similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this hard coded in the source codes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to say if the assembly that the Type came from has our public key, it is a "known resource".

@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants