Cherry-Pick File Sink & Bug Fixes for 1.6 Release #2852
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Why make this change? - This change solves issue #2576. We need to add the new `File Sink` properties that will later be used to send logs to `.txt` files locally. The properties need to have all the necessary components to be serialized and deserialized from the config file. ## What is this change? - This change adds the `File Sink` properties to the schema file. - Creates a new file `FileSinkConverter.cs` where the properties are serialized and deserialized. - Creates a new file `FileSinkOptions.cs` where the deserialized properties are turned to usable objects and adds the object to the `Telemetry` options. #### JSON Configuration Schema The configuration now supports the following structure under `runtime.telemetry`: ```json { "runtime": { "telemetry": { "file": { "enabled": true, "path": "/logs/dab-log.txt", "rolling-interval": "Day", "retained-file-count-limit": 7, "file-size-limit-bytes": 1048576 } } } } ``` ## How was this tested? - [ ] Integration Tests - [x] Unit Tests - [x] Manual Tests Tested that the newly created properties inside the config file are saved correctly as objects inside DAB and if the properties are written incorrectly, an exception is raised. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
…om the JWT token (#2344) ## Why make this change? - Fixes #2341 - The session context in SQL server is `read_only = 1` which prevents users from doing multiple requests on the same connection. - The row level security is not accurately implemented when using a JWT token. ## What is this change? Changes the session context from `read_only = 1` to `read_only = 0` to allow multiple requests to be done in the same connection, Creates a copy of the original 'roles' from the JWT token to use it on the SQL Filter Predicate to accurately implement row level security. ## How was this tested? - [ ] Integration Tests - [x] Unit Tests Updated `AuthorizationResolver` tests to ensure the original roles copy is working properly. ## Sample Request(s) Sample of a JWT token (only the relevant part) ``` { "aud": "api://ddcf6b31-5d01-407d-97cf-8efefc455d32", "iss": "https://sts.windows.net/9215c785-95c3-49b0-bdba-2062df5aedb5/", "roles": [ "user", "Allow_Customer_OPS025235", "Allow_Customer_OPS004095" ], "ver": "1.0" } ``` X-MS-API-ROLE: user before my change the extra 'roles' that do not match the X-MS-API-ROLE header would never reach the database context. With my change you can do things like this in SQL Predicates to filter out only subsets of the data: ``` CREATE FUNCTION dbo.ops_fact_order_Predicate(@CustomerNo varchar(max)) RETURNS TABLE WITH SCHEMABINDING AS RETURN SELECT 1 AS fn_securitypredicate_result WHERE @CustomerNo in ( select trim(replace(replace(replace([value], '"', ''), ']', ''), 'Allow_Customer_', '')) from STRING_SPLIT ( CAST(SESSION_CONTEXT(N'original_roles') as varchar(max)) , ',' , 0) where trim(replace(replace([value], '"', ''), ']', '')) like 'Allow_Customer%' ) CREATE SECURITY POLICY dbo.ops_fact_order_Policy ADD FILTER PREDICATE dbo.ops_fact_order_Predicate(CustomerNo) ON [gold_ops].[ops_fact_order]; ``` --------- Co-authored-by: KobeLenjou <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]> Co-authored-by: Ruben Cerna <[email protected]> Co-authored-by: RubenCerna2079 <[email protected]>
## Why make this change? - Closes issue #2578 - In order to complete the File Sink feature, the logic that sends the logs created by DAB to the file is required. ## What is this change? - This change implements the logic that sends all the logs from DAB to the file through the path the user requests. This is done with the help of Serilog which is a logging library that simplifies the creation of file sinks. - The `Startup.cs` program creates the Serilog logger pipeline and adds it as part of the services so that it is used later by the `Program.cs` to set the different loggers with the Serilog pipeline and allow the logs to be sent to the file sink. - We also deleted the `RollingIntervalMode.cs` since we discovered that Serilog has its own rolling interval enum class, which makes the one implemented in DAB obsolete. ## How was this tested? - [ ] Integration Tests - [X] Unit Tests Created tests that check if the services needed for the File Sink exist when the File Sink property is enabled. Also, created test to check if the file sink with the appropriate name is created when the property is enabled. ## Sample Request(s) <img width="917" height="136" alt="image" src="https://github.com/user-attachments/assets/a470844a-6ea3-4d05-9dfa-650f878c1c59" /> <img width="1873" height="948" alt="image" src="https://github.com/user-attachments/assets/1895eaae-0223-4d48-924e-c5275a98e3dc" />
## Why make this change? - Fixes issue #2577 - We want to allow the file sink properties to be configurable through the CLI command `dab configure`. ## What is this change? This change adds the file sink properties to the configure command and allows the user to change those properties through the CLI. It also ensures that the path property exists if file sink is enabled. - `ConfigOptions.cs`: Adds file sink properties to CLI command so that they can be configured by the user. - `ConfigGenerator.cs`: Writes the file sink properties to the config file, and errors out if the user tries to add an invalid value. - `RuntimeConfigValidator.cs`: Validates that `runtime.telemetry.file.path` is not empty or null if the file sink is enabled. ## How was this tested? - [ ] Integration Tests - [X] Unit Tests Added tests that ensure the configure commands and validation work. Also refactored the validation tests that followed the same pattern. ## Sample Request(s) --runtime.telemetry.file.enabled --runtime.telemetry.file.path --runtime.telemetry.file.rolling-interval --runtime.telemetry.file.retained-file-count-limit --runtime.telemetry.file.file-size-limit-bytes --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
anushakolan,
souvikghosh04,
neeraj-sharma2592,
sourabh1007,
vadeveka,
Alekhya-Polavarapu and
rusamant
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR cherry-picks several features and bug fixes for the 1.6 release, primarily implementing File Sink telemetry functionality and fixing authentication-related issues.
- Implements File Sink telemetry feature with Serilog integration for logging to files
- Fixes session context parameter handling in SQL Server by changing from read-only to read-write
- Preserves original role claims during JWT token authentication processing
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Adds File Sink configuration and Serilog logger setup |
| src/Service/Program.cs | Integrates Serilog provider into logger factory |
| src/Config/ObjectModel/FileSinkOptions.cs | Defines File Sink configuration options with defaults |
| src/Config/Converters/FileSinkConverter.cs | JSON serialization/deserialization for File Sink options |
| src/Core/Resolvers/MsSqlQueryExecutor.cs | Changes session context from read-only to read-write |
| src/Core/Authorization/AuthorizationResolver.cs | Preserves original roles claim during processing |
| Multiple test files | Adds comprehensive test coverage for File Sink functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/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). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why make this change?
What is this change?
Cherry-picked PRs:
How was this tested?