-
Notifications
You must be signed in to change notification settings - Fork 254
Add Serialization & Deserialization of Sink File Properties with CLI Commands #2752
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]>
Co-authored-by: RubenCerna2079 <[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 support for file-based telemetry sinks by extending configuration, CLI commands, and JSON schema.
- Introduces a new
FileSinkOptions
model andRollingIntervalMode
enum. - Updates
TelemetryOptions
andConfigGenerator
to incorporate file sink settings. - Expands the CLI (
AddTelemetryOptions
,ConfigureOptions
) and adds unit tests and JSON schema entries for file sink parameters.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/Config/ObjectModel/FileSinkOptions.cs | New record for configuring file sink telemetry. |
src/Config/ObjectModel/FileSinkRollingIntervalMode.cs | New enum for file sink rolling intervals. |
src/Config/ObjectModel/TelemetryOptions.cs | Extended to include File property. |
src/Cli/ConfigGenerator.cs | Wired file sink options into runtime config builder. |
src/Cli/Commands/AddTelemetryOptions.cs | Added CLI flags for file sink telemetry settings. |
src/Cli/Commands/ConfigureOptions.cs | Exposed file sink flags on dab configure command. |
src/Cli.Tests/ConfigureOptionsTests.cs | Added tests for file sink CLI configuration. |
schemas/dab.draft.schema.json | Added JSON schema for file sink under runtime.telemetry.file . |
src/Cli/Properties/launchSettings.json | Updated default CLI launch arguments for testing. |
Comments suppressed due to low confidence (1)
src/Cli.Tests/ConfigureOptionsTests.cs:886
- The tests reference
RollingInterval
, but the enum is namedRollingIntervalMode
. Update references toRollingIntervalMode.Hour
(and similarly for Day/Week).
[DataRow(RollingInterval.Hour, DisplayName = "Update rolling interval to Hour.")]
/// Represents the rolling interval options for file sink. | ||
/// The time it takes between the creation of new files. | ||
/// </summary> | ||
[JsonConverter(typeof(JsonStringEnumConverter))] |
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 default JsonStringEnumConverter will serialize enum values as "Hour"/"Day"/"Week", but the JSON schema expects lowercase strings ("hour","day","week"). Consider using JsonStringEnumConverter with a camel‐case naming policy (e.g. new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)) or adjust the schema to match.
[JsonConverter(typeof(JsonStringEnumConverter))] | |
[JsonConverter(typeof(JsonStringEnumConverter), JsonNamingPolicy.CamelCase)] |
Copilot uses AI. Check for mistakes.
/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). |
@@ -68,6 +78,26 @@ 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 File Sink telemetry should be enabled. This flag is optional and default value is false. | |||
[Option("file-sink-enabled", Default = CliBool.False, Required = false, HelpText = "Enable/disable file sink telemetry logging. Default: false (boolean).")] |
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.
For any of these properties with defaults, if the default value is being used, but was not supplied by the user, does the property end up writing back out to the config?
string? Path = "/logs/dab-log.txt", | ||
RollingIntervalMode? RollingInterval = RollingIntervalMode.Day, | ||
int? RetainedFileCountLimit = 1, | ||
int? FileSizeLimitBytes = 1048576) |
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.
if we try to make FileSinkOptions with values below the minimum in the schema, do we end up with an invalid config, or do we error out first?
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.
Can we add some kind of a test to validate the scenario?
Why make this change?
This change solves issue #2749.
What is this change?
CLI Commands Added
The following new CLI configure options are now available:
--runtime.telemetry.file.enabled - Enable/disable file sink telemetry (default: false)
--runtime.telemetry.file.path - File path for telemetry logs (default: "/logs/dab-log.txt")
--runtime.telemetry.file.rolling-interval - Rolling interval: hour, day, week (default: day)
--runtime.telemetry.file.retained-file-count-limit - Number of retained files (default: 1, minimum: 1)
--runtime.telemetry.file.file-size-limit-bytes - Max file size in bytes (default: 1048576, minimum: 1)
JSON Configuration Schema
The configuration now supports the following structure under
runtime.telemetry
:How was this tested?
Sample Request(s)