Cherry-Picking Commits for 1.5 #2689
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
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
ravishetye,
neeraj-sharma2592,
sourabh1007 and
sezal98
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 all commits for milestone 1.5 into the release branch, introducing entity-level caching support, enhancing health‐check serialization, and updating pipeline restore steps.
- Add CLI flags, runtime model, converters, tests, and schema entries for
cache.enabled/cache.ttl - Extend
RuntimeHealthCheckConfigto parse and emitcache-ttl-seconds - Replace
NuGetCommand@2withDotNetCoreCLI@2restore steps in pipeline templates
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/Utils.cs | Introduce ConstructCacheOptions for parsing cache flags |
| src/Cli/ConfigGenerator.cs | Wire Cache option into entity creation/update |
| src/Cli/Commands/*Options.cs | Add cacheEnabled and cacheTtl parameters |
| src/Cli.Tests/* | Add tests and snapshots for entity caching |
| src/Config/Converters/RuntimeHealthOptionsConvertorFactory.cs | Parse and write new cache-ttl-seconds field |
| schemas/dab.draft.schema.json | Add JSON schema entry for cache-ttl-seconds |
| src/Config/HealthCheck/DatasourceHealthCheckConfig.cs | Adjust constructor parameter naming |
| src/Config/DatabasePrimitives/DatabaseObject.cs | Make SourceDefinition property virtual |
| .pipelines/** | Switch pipeline restore tasks to DotNetCoreCLI@2 |
Comments suppressed due to low confidence (2)
src/Config/DatabasePrimitives/DatabaseObject.cs:58
- Making
SourceDefinitionvirtual is a breaking API change—ensure this is documented in the changelog and that any derived types override or delegate correctly.
public virtual SourceDefinition SourceDefinition
.pipelines/templates/build-pipelines.yml:65
- Indentation and dash prefix appear inconsistent compared to surrounding steps; verify YAML syntax to prevent pipeline parse errors.
- task: DotNetCoreCLI@2
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
reviewed
May 20, 2025
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
aaronburtle
approved these changes
May 29, 2025
## Why make this change? Resolves #2649 ## What is this change? The pipelines are failing due to udpate from ubutu 24.04. We are using the latest version in our yamls hence, this change needs to be handled. <img width="527" alt="image" src="https://github.com/user-attachments/assets/b1f14841-ea20-4b18-8b17-b9f1c43b4022" /> The error message we are seeing is due to the fact that default Ubuntu 24.04+ image on Microsoft-hosted agents does not include Mono, which is required for the NuGet client that powers NuGetCommand@2 in our pipeline. Microsoft recommends migrating to using the NuGetAuthenticate@1 task and .NET CLI commands instead of the older NuGetCommand@2 task, which relies on Mono. Steps to Fix the Issue: - Remove NuGetCommand@2 task: Since this task requires Mono (which is no longer available by default on Ubuntu 24.04+), we should remove it and replace it with .NET CLI commands for restoring the NuGet packages. - Add .NET CLI restore command: Instead of using the NuGetCommand@2 task, use the DotNetCoreCLI@2 task to restore NuGet packages. - Update NuGet authentication: Ensure that you're using NuGetAuthenticate@1 before the restore task to handle authentication. ## How was this tested? The pipelines are running as expected. --------- Co-authored-by: sezalchug <[email protected]>
## Why make this change? Resolved #2645 ## What is this change? We need to take the plural value for the creating the http payload for the graphql request. Further we need to camel case this value instead of using the objects value which I was currently using ## How was this tested? Local Testing 1. Type.Plural not provided and entity name is not the same as table name 2. Type.Plural not provided and entity name is caps 3. Type.Plural not provided and entity name is all small 4. source.objects is provided as `dbo.books` 5. Type.Plural provided but in all small case 6. Type.Plural provided in all caps ## Sample Request(s) <img width="310" alt="image" src="https://github.com/user-attachments/assets/26c35c70-34a0-40d9-b344-90d7c31d117f" /> <img width="234" alt="image" src="https://github.com/user-attachments/assets/b3385e59-c9ee-4ad7-8fdb-2555bfaf16ed" /> --------- Co-authored-by: sezalchug <[email protected]>
…#2617) ## Why make this change? - Closes #2554 - Enhances OTEL instrumentation with custom traces and metrics for the REST APIs ## What is this change? This PR enhances the OTEL instrumentation for the REST APIs by adding custom traces and metrics. I have removed ASP NET Core standard instrumentation since it does not provide great value given the custom nature of the webservice. I have written two main Helper classes: `TelemetryMetricsHelper` and `TelemetryTracesHelper` to provide a single point of management for custom traces and metrics. Metrics can be filtered for `status_code`, `api_type`, `endpoint` and `method`. I have also fixed the loggings which are now sent to the configured OTEL endpoint. ### Logs  ### Metrics  ### Traces    ## How was this tested? - [ ] Integration Tests - [ ] Unit Tests ## Sample Request(s) To test everything locally I recommend using [this repo](https://github.com/tommasodotNET/dab-workbench) that allows to run the local build of the dab cli and send metrics to the .NET Aspire OTEL endoint. --------- Co-authored-by: Aaron Powell <[email protected]> Co-authored-by: RubenCerna2079 <[email protected]> Co-authored-by: aaronburtle <[email protected]> Co-authored-by: Ruben Cerna <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
## Why make this change? Internal Issue Resolved ## What is this change? Created a check before creating the HttpClient where the URI would be validated Conditions 1. It ensures the URI is absolute. 2. It checks for valid HTTP/HTTPS schemes. 3. Disallow empty hostnames Working as expected Code QL resolution would be checked after merging and running the pipelines --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: aaronburtle <[email protected]>
## Why make this change? closes #2603 ## What is this change? Includes caching options with the `add` and `update` verbs for the CLI. These verbs are used to add or update new entities in a config file, and will now include support for the `cache` entry in those entities. ## How was this tested? Current test suite. ## Sample Request(s) You can test with the CLI by using the `add` or `update` verb along with the flags `--cache.enabled` and `--cache.ttl`, default values are false and 5. --------- Co-authored-by: Ruben Cerna <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
## Why make this change? Closes [#2534](#2534) ## What is this change? This PR involves creating another parameter in the runtime config which defines the cache-ttl-seconds for the health endpoint. + I am using the IMemoryCache to define the cache which has the CacheKey: `HealthCheckResponse` in ComprehensiveResponseWriter and saves the serialized version of the response in the cache. + The changes in Convertor Factory are in respect to the serialization and deserialization of this attribute appropriately. We consider default as 5 seconds in case not provided in the config file. And in case this is 0, caching is disabled. + Added timestamp to the health check report ## How was this tested? - [x] Integration Tests - [x] Unit Tests ## Sample Request(s) Call this CURL curl --request GET \ --url http://localhost:5000/health \ --header 'User-Agent: insomnia/10.3.0' Define the cache value as 5. Then in case of hitting the request without any delay, we get the same response back from the cache. In case hitting after the delay, we get a new response. --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
…h check (#2667) ## Why make this change? Resolves #2662 ## What is this change? 1. Created the HttpUtilities with HttpClient already instantiated and basic details filled from Startup.cs 2. Using this httpClient, call the REST and GraphQL requests with relative URI 3. Created a function in DatabaseObject to access mapping dict as it is an abstract class which was not allowed to be mocked. 4. Test cases for both REST and GraphQL 5. Rest Test case: Mock the client to return a 200 response when we receive a GET request with specific URI. Validate the error message from HttpUtilities should be null. 6. GraphQL Test case: Mock the client to return a 200 response when we get a POST request with /graphql URI and content payload. Mock the Dabase Object with columns in the entity map to only return the db object when a certain entity is called for. ## How was this tested? - [ ] Integration Tests - [x] Unit Tests ## Sample Request(s) Requests are running as expected. Test cases added for Rest and Graphql --------- Co-authored-by: sezalchug <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
…2673) This closes #2642 This PR enhances the OTEL instrumentation for the GraphQL APIs by adding custom traces and metrics. Metrics can be filtered for `status_code`, `api_type`, `endpoint` and `method`. - [X] Local Testing - [ ] Integration Tests - [ ] Unit Tests All of the tests were done locally to check if the log information that was provided was correct, for both scenarios in which the query gave the proper information or when an exception was raised. Another thing that was tested is that when we open GraphQL it would send a few requests called `Introspection Queries` used to ensure that GraphQL is working properly. However, we do not want the user to see these requests as part of the total count as this is done automatically, which may confuse the users.    - Clone the following repo `https://github.com/tommasodotNET/dab-workbench.git` - Run your DAB version in CLI so the files from the `out` folder are created, and make sure to stop it before running the DAB Workbench since both cannot be running at the same time. - Find the path to the `Microsoft.DataApiBuilder.exe`, which should look something like `<PATH_TO_REPO>\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.exe` - Copy the path of the `.exe` file and paste it in the file `/DABWorkbench.AppHost/Program.cs` in the variable `dabCLIPath` which is found in line 3 as follows: `var dabCLIPath = @"<PATH_TO_REPO>\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.exe";` - Now you should be able to run DAB Workbench with your version of DAB. --------- Co-authored-by: Tommaso Stocchi <[email protected]> Co-authored-by: Jerry Nixon <[email protected]> Co-authored-by: Ruben Cerna <[email protected]>
## Why make this change? Closes #2604 ## What is this change? Inside of the `SqlQueryStructure`, there is a private constructor, that all of the public constructors will call. This private constructor offers a single point where all of the queries will bottleneck, and therefore we add the cache control information to the query structure at this point. Then when we are in the `SqlQueryEngine`, we can check for this cache control information and handle the query appropriately. The cache control options can be found here: #2253 and here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Cache-Control#request_directives But for reference, they are `no-cache`, `no-store`, `only-if-cached`, which will mean we do not get from the cache, we do not store in the cache, and if there is a cache miss we return gateway timeout, respectively. ## How was this tested? Run against test suite. ## Sample Request(s) To test you need to include the relevant cache headers in the request, "no-cache", "no-store", or "only-if-cached" --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]> Co-authored-by: RubenCerna2079 <[email protected]>
The CosmosSqlMetdataProvider uses a standard dictionary although the metadata provider is accessed and updated concurrently. This causes the issue in #2694. I have updated the implementation to use a ConcurrentDictionary. Fixes #2694 Co-authored-by: Aniruddh Munde <[email protected]>
## Why make this change? This change solves the bug #2690 ## What is this change? We update the `Microsoft.Data.SqlClient` package from version 5.2.0 to version 5.2.3 in the `Directory.Packages.props` file. As the bug is a known error from the current version we are using. ## How was this tested? - [ ] Integration Tests - [ ] Unit Tests ## Sample Request(s) --------- Co-authored-by: Ruben Cerna <[email protected]>
- Handle SSL validation failures in Health check when run in untrusted cert and allow option to run with self signed cert - Allow docker container to run on any port internally and allow health check API to use the internal port - Added support for specifying self signed cert (true/false) in environment variable- `USE_SELF_SIGNED_CERT` - Added support for running container on different ports as specified in environment variable- `ASPNETCORE_URLS=http://+:5000` - The internal port is usually 5000 and is set with environment variable `ASPNETCORE_URLS` which is where is the runtime is started - This port is not required to be changed, however, it can be overridden by setting the environment variable `ASPNETCORE_URLS` to `http://+:<internal_port>` - When using `docker run`, the `--publish <external_port>:<internal_port>` must match the internal port, as specified in the `ASPNETCORE_URLS` environment variable - For orchestrated environments, like- ACI, AKS etc, the internal port is set in the same way in environment variable, additionally, the external port is set in the service configuration (or as applicable to the specific orchestration tool) Running on same external and internal port. This was the existing way. ``` docker run --name dab-dev --publish 5000:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ``` Running on different external and internal ports. With this, we can expose an external port which is different than the internal container port. ``` docker run --name dab-dev --publish 1234:5000 --detach --mount type=bind,source=$(pwd)/dab-config.json,target=/App/dab-config.json,readonly dab-dev:26may25 ```  ``` GET http://localhost:1234/health Accept: application/json --------- Co-authored-by: Souvik Ghosh <[email protected]> Co-authored-by: Aniruddh Munde <[email protected]>
RubenCerna2079
force-pushed
the
dev/rubencerna/cherry-pick-1.5
branch
from
68e903a to
32f1092
Compare
|
/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?
This change is made in order to add all of the commits for milestone 1.5 into its respective branch.
What is this change?
This change cherry-picks all of the commits that were added after the first release candidate.
Cherry-picked commits:
[Bug] Fix Health request for GraphQL endpoint #2657
2554 Enhance REST OTEL instrumentation with custom metrics and traces #2617
Resolve SSRF in HealhCheck #2659
Include cache options in the add and update verbs of CLI #2655
Caching response from Health Endpoint #2633
[Health]: Test Case for Rest and GraphQL Requests to DAB during health check #2667
Enhance GraphQL OTEL instrumentation with custom metrics and traces #2673
Add header support for caching #2650
Fixed CosmosSqlMetadataProvider concurrency issue. #2695
Upgrade Microsoft.Data.SqlClient to New Version #2702
HttpClient fixes for Container port and SSL validation #2688
How was this tested?
Sample Request(s)