Addition & Deserialization of Azure Log Analytics Properties #2727
Conversation
Co-authored-by: RubenCerna2079 <[email protected]>
Copilot
AI
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
anushakolan,
souvikghosh04,
ravishetye,
neeraj-sharma2592,
sourabh1007,
vadeveka,
Alekhya-Polavarapu and
rusamant
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Azure Log Analytics telemetry by extending the DAB schema, configuration objects, and deserialization handling while updating related tests accordingly.
- Introduces AzureLogAnalyticsOptions in Startup.cs and a corresponding configuration method.
- Updates TelemetryOptions and JSON schema to include Azure Log Analytics properties.
- Adds unit tests and configuration tests covering the new telemetry options.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Adds Azure Log Analytics options initialization and configuration logic. |
| src/Service.Tests/UnitTests/RuntimeConfigLoaderJsonDeserializerTests.cs | Updates test JSON to include the "azure-log-analytics" property. |
| src/Service.Tests/Configuration/AzureLogAnalyticsConfigurationTests.cs | Provides tests for Azure Log Analytics configuration setup. |
| src/Config/ObjectModel/TelemetryOptions.cs | Extends the telemetry options record with Azure Log Analytics support. |
| src/Config/ObjectModel/AzureLogAnalyticsOptions.cs | Introduces a new record for defining Azure Log Analytics options. |
| src/Config/ObjectModel/AzureLogAnalyticsAuthOptions.cs | Introduces a new record for Azure Log Analytics authentication options. |
| schemas/dab.draft.schema.json | Adds schema definitions for configuring Azure Log Analytics telemetry. |
Comments suppressed due to low confidence (1)
src/Service.Tests/Configuration/AzureLogAnalyticsConfigurationTests.cs:65
- [nitpick] The test method name references 'OpenTelemetry' even though it is defined in the Azure Log Analytics tests; consider renaming it to 'TestAzureLogAnalyticsServicesEnabled' for consistency.
public void TestOpenTelemetryServicesEnabled()
|
|
||
| if (AzureLogAnalyticsOptions.Auth is null) | ||
| { | ||
| _logger.LogWarning("Logs won't be sent to Azure Log Analytics because the Authorization options are not avaiable in the runtime config."); |
There was a problem hiding this comment.
Correct the misspelling of 'avaiable' to 'available' in the log message for clarity.
| _logger.LogWarning("Logs won't be sent to Azure Log Analytics because the Authorization options are not avaiable in the runtime config."); | |
| _logger.LogWarning("Logs won't be sent to Azure Log Analytics because the Authorization options are not available in the runtime config."); |
Copilot uses AI. Check for mistakes.
RubenCerna2079
changed the title
| /// <summary> | ||
| /// Represents the options for configuring Azure Log Analytics. | ||
| /// </summary> | ||
| public record AzureLogAnalyticsOptions( |
There was a problem hiding this comment.
Are all of the options in this and the other classes required? If not, you might wan to follow the pattern used in the other options classes, where a default value is maintained as a property, as well as a bool that indicates if the user provided the property or not. That way the default value can be set when the property is excluded, and we can avoid writing properties back to the config file if that property wasn't included by the user.
There was a problem hiding this comment.
Updated both classes to follow the established pattern. Added UserProvided flags for properties with defaults (Enabled, LogType, FlushIntervalSeconds) and constructor logic to handle default values. The auth properties remain nullable since they don't have meaningful defaults. Changes in commit 5dac9fc.
…erProvided flags Co-authored-by: aaronburtle <[email protected]>
| } | ||
| else | ||
| { | ||
| this.Enabled = false; |
There was a problem hiding this comment.
How come we are not using a default value for Enabled?
There was a problem hiding this comment.
We are using a default value for Enabled. In the constructor (line 62), when Enabled is not provided by the user, it defaults to false. The property is declared as nullable to support the CLI merge config pattern, but gets assigned the default value during construction.
There was a problem hiding this comment.
I mean the other default values have their own constant property, why is this one just assigned on its own in the constructor?
| /// </summary> | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.Always)] | ||
| [MemberNotNullWhen(true, nameof(Enabled))] | ||
| public bool UserProvidedEnabled { get; init; } = false; |
There was a problem hiding this comment.
Thanks for adding the UserProvided flags, but for these to actually be used will require an implementation that extends JsonConverterFactory so that the logic for writing can use these flags in order to determine if they should be written to the config file or not.
There was a problem hiding this comment.
Is this planned for another PR? I think we also need to have the configure command from the CLI that configures the runtime settings to include logic for these new properties as well.
There was a problem hiding this comment.
Added the JsonConverterFactory implementation for AzureLogAnalyticsOptions. The converter only serializes properties when their corresponding UserProvided flags are true, preventing default values from polluting the config file. The converter is registered in RuntimeConfigLoader and includes tests to verify the behavior. Changes in commit 066e6ff.
…ided flags Co-authored-by: aaronburtle <[email protected]>
| /// <inheritdoc/> | ||
| public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| return new AzureLogAnalyticsOptionsConverter(); |
There was a problem hiding this comment.
why are we not passing along _replaceEnvVar here?
| /// <summary> | ||
| /// Defines how DAB reads and writes Azure Log Analytics options. | ||
| /// </summary> | ||
| internal class AzureLogAnalyticsOptionsConverterFactory : JsonConverterFactory |
There was a problem hiding this comment.
don't we need a constructor that can take a bool for replacing environment variables?
| /// Uses default deserialize. | ||
| /// </summary> | ||
| /// <exception cref="JsonException">Thrown when improperly formatted Azure Log Analytics options are provided.</exception> | ||
| public override AzureLogAnalyticsOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) |
There was a problem hiding this comment.
will simply calling the deserialize function from JsonSerializer be enough here or do we need explicity read logic?
Why make this change?
Fixes issue #2726
What is this change?
Adds the properties needed to support Azure Log Analytics to the DAB schema and configuration objects as well as the deserialization of those properties.
How was this tested?