Cherry-Picking Commits for 1.5 #2689
Open
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? 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]>
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
Comment on lines
+411
to
+413
| "cache-ttl-seconds": { | ||
| "type": "integer", | ||
| "description": "Time to live in seconds for the Comprehensive Health Check Report cache entry.", |
There was a problem hiding this comment.
[nitpick] CLI flag uses --cache.ttl but JSON schema uses cache-ttl-seconds; consider unifying or documenting the translation to avoid confusion for API consumers.
Suggested change
| "cache-ttl-seconds": { | |
| "type": "integer", | |
| "description": "Time to live in seconds for the Comprehensive Health Check Report cache entry.", | |
| "cache.ttl": { | |
| "type": "integer", | |
| "description": "Time to live (in seconds) for the Comprehensive Health Check Report cache entry. Matches the CLI flag '--cache.ttl'.", |
Copilot uses AI. Check for mistakes.
|
/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.
How was this tested?
Sample Request(s)