-
Notifications
You must be signed in to change notification settings - Fork 254
Adding 'add-telemetry' Options to CLI for Azure Log Analytics #2772
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: RubenCerna2079 <[email protected]>
…erProvided flags Co-authored-by: aaronburtle <[email protected]>
…ided flags Co-authored-by: aaronburtle <[email protected]>
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.
Pull Request Overview
This PR adds CLI support for Azure Log Analytics telemetry configuration in the add-telemetry
command. The changes enable users to configure Azure Log Analytics properties such as workspace ID, DCR immutable ID, DCE endpoint, log type, and flush interval through command-line options.
Key changes include:
- Added new CLI options for Azure Log Analytics configuration parameters
- Updated constructor parameter naming from camelCase to PascalCase for consistency
- Fixed serialization logic to only include auth options when user-provided values exist
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
src/Cli/Commands/AddTelemetryOptions.cs | Added 6 new CLI options for Azure Log Analytics configuration |
src/Cli/ConfigGenerator.cs | Integrated validation and object creation for Azure Log Analytics options |
src/Config/ObjectModel/AzureLogAnalyticsOptions.cs | Updated constructor parameters to PascalCase naming convention |
src/Config/ObjectModel/AzureLogAnalyticsAuthOptions.cs | Updated constructor parameters to PascelCase naming convention |
src/Config/Converters/AzureLogAnalyticsOptionsConverterFactory.cs | Fixed serialization condition for auth options |
src/Service.Tests/Configuration/ConfigurationTests.cs | Added test helper method and import |
src/Cli.Tests/ValidateConfigTests.cs | Added validation test for Azure Log Analytics without auth |
src/Cli.Tests/Cli.Tests.csproj | Updated BOM marker |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
The issue that this PR contains should at least mention in brief what this PR is trying to achieve. Example from parent issue that can be added:
|
@@ -1,4 +1,4 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Project Sdk="Microsoft.NET.Sdk"> |
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.
What is the change here?
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.
I did not make any changes to this file
@@ -27,23 +27,23 @@ public record AzureLogAnalyticsAuthOptions | |||
public string? DceEndpoint { get; init; } | |||
|
|||
[JsonConstructor] | |||
public AzureLogAnalyticsAuthOptions(string? workspaceId = null, string? dcrImmutableId = null, string? dceEndpoint = null) | |||
public AzureLogAnalyticsAuthOptions(string? WorkspaceId = null, string? DcrImmutableId = null, string? DceEndpoint = null) |
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.
why are we changing the casing here? function parameters usually follow camelCasing
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.
I saw that it is the format that is used for the other Telemetry Options, if we want it to follow camelCasing we need to change the other options as well
@@ -49,38 +49,38 @@ public record AzureLogAnalyticsOptions | |||
public int? FlushIntervalSeconds { get; init; } | |||
|
|||
[JsonConstructor] | |||
public AzureLogAnalyticsOptions(bool? enabled = null, AzureLogAnalyticsAuthOptions? auth = null, string? logType = null, int? flushIntervalSeconds = null) | |||
public AzureLogAnalyticsOptions(bool? Enabled = null, AzureLogAnalyticsAuthOptions? Auth = null, string? LogType = null, int? FlushIntervalSeconds = null) |
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.
same question here: why is this change needed?
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.
I saw that it is the format that is used for the other Telemetry Options, if we want it to follow camelCasing we need to change the other options as well
public string? AzureLogAnalyticsLogType { get; } | ||
|
||
// Specify the flush interval in seconds for Azure Log Analytics resource to which telemetry data should be sent. | ||
[Option("azure-log-analytics-flush-interval-seconds", Required = false, HelpText = "Flush Interval Seconds for Azure Log Analytics for specifying time it takes to send telemetry data")] |
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.
[Option("azure-log-analytics-flush-interval-seconds", Required = false, HelpText = "Flush Interval Seconds for Azure Log Analytics for specifying time it takes to send telemetry data")] | |
[Option("azure-log-analytics-flush-interval-seconds", Required = false, HelpText = "Flush Interval in seconds for Azure Log Analytics for specifying time it takes to send telemetry data")] |
public int? AzureLogAnalyticsFlushIntervalSeconds { get; } | ||
|
||
// Specify the Workspace ID for Azure Log Analytics resource to which telemetry data should be sent. | ||
[Option("azure-log-analytics-auth-workspace-id", Required = false, HelpText = "Workspace ID for Azure Log Analytics to find table to send telemetry data")] |
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.
the helptext needs to be updated to mention WorkspaceID. Looks like a copy paste issue
|
||
// Specify the DCR Immutable ID for Azure Log Analytics resource to which telemetry data should be sent. | ||
[Option("azure-log-analytics-auth-dcr-immutable-id", Required = false, HelpText = "DCR Immutable ID for Azure Log Analytics to find table to send telemetry data")] | ||
public string? AzureLogAnalyticsDcrImmutableId { get; } |
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.
the help text needs to be updated to mention DCR Immutable ID. Looks like a copy paste issue
} | ||
|
||
/// <summary> | ||
/// Represents a JSON string for the telemetry section of the config, with Application Insights enabled and a specified connection string. |
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.
This summary needs to be updated. Typo.
""telemetry"": {}"; | ||
|
||
/// <summary> | ||
/// Represents a JSON string for the empty telemetry section of the config. |
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.
Summary needs to be updated.
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.
Overall looks fine, but needs to fix description, title to mention this is adding add-telemetry options to the CLI instead of saying "Add CLI Azure Log Analytics".
@@ -68,6 +80,30 @@ public AddTelemetryOptions( | |||
[Option("otel-service-name", Default = "dab", Required = false, HelpText = "(Default: dab) Headers for Open Telemetry for telemetry data")] | |||
public string? OpenTelemetryServiceName { get; } | |||
|
|||
// To specify whether Azure Log Analytics telemetry should be enabled. This flag is optional and default value is false. | |||
[Option("azure-log-analytics-enabled", Default = CliBool.False, Required = false, HelpText = "(Default: false) Enable/Disable Azure Log Analytics")] |
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.
The correct syntax should be dab configure --runtime.telemetry.azure-log-analytics.enabled true
please.
Why make this change?
This change closes issue #2771.
What is this change?
This change allows the users to use the CLI command
add-telemetry
for the Azure Log Analytics properties.How was this tested?
Sample Request(s)