-
Notifications
You must be signed in to change notification settings - Fork 763
Follow-up telemetry updates #9052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5eb6344
790ab08
968728c
7e02ee8
0ce32de
bc934c5
6429bb8
cb390b0
83a2de6
863f95b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -860,14 +860,10 @@ public void UpdateTelemetryProperties() | |
| { | ||
| var properties = new List<ComponentTelemetryProperty> | ||
| { | ||
| 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())) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| }; | ||
|
|
||
| foreach (var resourceTypeGroup in _resourceByName.Values.GroupBy(r => r.ResourceType)) | ||
| { | ||
| properties.Add(new ComponentTelemetryProperty($"{TelemetryPropertyKeys.ResourceType}.{resourceTypeGroup.Key}", new AspireTelemetryProperty(resourceTypeGroup.Count(), AspireTelemetryPropertyType.Metric))); | ||
| } | ||
|
|
||
| TelemetryContext.UpdateTelemetryProperties(properties.ToArray()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -495,11 +495,8 @@ public class StructuredLogsPageState | |
| public void UpdateTelemetryProperties() | ||
| { | ||
| TelemetryContext.UpdateTelemetryProperties([ | ||
| new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsSelectedApplication, new AspireTelemetryProperty(PageViewModel.SelectedApplication.Id?.ToString() ?? string.Empty)), | ||
| new ComponentTelemetryProperty(TelemetryPropertyKeys.StructuredLogsSelectedLogLevel, new AspireTelemetryProperty(PageViewModel.SelectedLogLevel.Id?.ToString() ?? string.Empty, AspireTelemetryPropertyType.UserSetting)), | ||
| 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)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you're doing a ToString here but not for |
||
| ]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Aspire.Dashboard.Model; | ||
| using Aspire.Hosting.Dashboard; | ||
|
|
||
| namespace Aspire.Dashboard.Telemetry; | ||
|
|
||
| public static class TelemetryPropertyValues | ||
| { | ||
| private const string CustomResourceCommand = "custom-command"; | ||
| private const string CustomResourceType = "custom-resource-type"; | ||
|
|
||
| public static string GetCommandNameTelemetryValue(string commandName) | ||
| { | ||
| return KnownResourceCommands.IsKnownCommand(commandName) | ||
| ? commandName | ||
| : CustomResourceCommand; | ||
| } | ||
|
|
||
| public static string GetResourceTypeTelemetryValue(string resourceType) | ||
| { | ||
| return KnownResourceTypes.IsKnownResourceType(resourceType) | ||
| ? resourceType | ||
| : CustomResourceType; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,4 +8,12 @@ internal static class KnownResourceTypes | |
| public const string Executable = "Executable"; | ||
| public const string Project = "Project"; | ||
| public const string Container = "Container"; | ||
|
|
||
| // 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"]; | ||
|
Comment on lines
+12
to
+13
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have to maintain an allow list of resources
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this hard coded in the source codes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
|
|
||
| public static bool IsKnownResourceType(string resourceType) | ||
| { | ||
| return s_builtInResources.Contains(resourceType); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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