Health Endpoint for DAB Engine #2515
Conversation
sezal98
requested review from
Aniruddh25,
aaronburtle,
abhishekkumams,
RubenCerna2079,
ravishetye,
rohkhann,
neeraj-sharma2592 and
sourabh1007
as code owners
sezal98
changed the title
sezal98
changed the title
| @@ -44,6 +46,7 @@ public Entity( | |||
| Dictionary<string, string>? Mappings, | |||
| Dictionary<string, EntityRelationship>? Relationships, | |||
| EntityCacheOptions? Cache = null, | |||
| DabHealthCheckConfig? Health = null, | |||
There was a problem hiding this comment.
nit: the name could be simplified to just say HealthCheck in alignment with other options like Mappings. Since this is already in the context of Dab and its a config, the prefix Dab and suffix Config are not needed.
There was a problem hiding this comment.
why was this resolved? i don't see an update.
|
Can we have some unit tests to verify the changes in the current PR? Also add what changes are missing here that will be added in the subsequent PRs. |
sezal98
changed the title
|
/azp run |
| /// <summary> | ||
| /// The version of the service. | ||
| /// </summary> | ||
| public string? Version { get; set; } |
There was a problem hiding this comment.
Why would this ever be null? Wont we have a version always
| /// <summary> | ||
| /// The name of the dab service. | ||
| /// </summary> | ||
| public string? AppName { get; set; } |
There was a problem hiding this comment.
Same: AppName should always be present
| /// <summary> | ||
| /// The configuration details of the dab service. | ||
| /// </summary> | ||
| public DabConfigurationDetails? DabConfigurationDetails { get; set; } |
There was a problem hiding this comment.
Dab cannot start without configuration, this has to be non-null
| namespace Azure.DataApiBuilder.Config.ObjectModel | ||
| { | ||
| /// <summary> | ||
| /// The health report of the DAB Enigne. |
There was a problem hiding this comment.
nit: Copy paste issue, fix description
| /// </summary> | ||
| public record DabConfigurationDetails | ||
| { | ||
| public bool Rest { get; init; } |
There was a problem hiding this comment.
Is bool sufficient or do we need to provide the complete details of the Rest runtime options? same for GraphQL
|
|
||
| [JsonPropertyName("threshold-ms")] | ||
| public int? ThresholdMs { get; set; } // (Default: 10000ms) | ||
| public string Role { get; set; } = "*"; // "roles": ["anonymous", "authenticated"] (Default: *) |
There was a problem hiding this comment.
nit: be more descriptive of what this Role is for, could be RoleAuthorizedToViewHealth e.g. Set JsonPropertyName to be Role
| /// Creates a JSON response for the health check endpoint using the provided health report. | ||
| /// If the response has already been created, it will be reused. | ||
| /// </summary> | ||
| public class HealthReportResponseWriter |
There was a problem hiding this comment.
nit: the file name should be HealthReportResponseWriter
| // Dependencies | ||
| private ILogger? _logger; | ||
| private RuntimeConfigProvider _runtimeConfigProvider; | ||
| private OriginalHealthReportResponseWriter _originalHealthReportResponseWriter; |
There was a problem hiding this comment.
Not sure why we need a new ResponseWriter. Couldnt we just check the config flag and decide whether to include the additional content or not?
| @@ -12,7 +12,12 @@ namespace Azure.DataApiBuilder.Config.ObjectModel; | |||
| /// <param name="DatabaseType">Type of database to use.</param> | |||
| /// <param name="ConnectionString">Connection string to access the database.</param> | |||
| /// <param name="Options">Custom options for the specific database. If there are no options, this could be null.</param> | |||
| public record DataSource(DatabaseType DatabaseType, string ConnectionString, Dictionary<string, object?>? Options) | |||
| /// <param name="Health">Health check configuration for the database. In case null, follow old format of health check.</param> | |||
There was a problem hiding this comment.
shouldn't this be updated? since we now want to have both old format on the original address and new format on\health?
| // The moniker or simple name of the data source to be checked. | ||
| // Required when there is a multiple data source scenario. | ||
| // TODO: Add validity support for when multiple data sources | ||
| public string? Moniker { get; set; } |
There was a problem hiding this comment.
I thought we are updating this and not use Moniker.
|
|
||
| // The query to be executed to check the health of the data source. | ||
| // "query: "SELECT TOP 1 1" | ||
| public string? Query { get; set; } |
There was a problem hiding this comment.
We are also not going to have Query as discussed
|
The PR description needs to be updated. |
Why make this change?
This PR includes structural End to end changes for Health Endpoint.
It involved keeping both pipelines steady for original and enhanced health check based on a config flag in the runtime Config.
Resolves #2502
What is this change?
Created a new parameter in RuntimeConfig for Health which would include if the new enhanced version of health check is enabled or not.
New parameter of Health in DataSource which would provide info regarding which query to run on DB and under what threshold.
Created a class DabHealthCheckReport which would include details of status, version and app name like before. But would also be enhanced now to get details of config supplied and health of entities and data source.
TODO
How was this tested?
Currently tested on local. Would be improved further with UTs.
Sample Request(s)
Enhanced Format
Original Format