Import CPU by wall clock - #5793
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5793 +/- ##
==========================================
- Coverage 78.58% 78.19% -0.39%
==========================================
Files 1016 1018 +2
Lines 36898 37108 +210
Branches 5610 5674 +64
==========================================
+ Hits 28996 29017 +21
- Misses 6495 6699 +204
+ Partials 1407 1392 -15 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Test-mode validation, unbounded fan-out, incomplete-job handling, and metric aggregation can produce unsafe or inaccurate behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds in-memory $import workload generation and timing metrics for import CPU profiling.
Changes:
- Adds an embedded 1,000-resource dataset and synthetic processing-job fan-out.
- Tracks wall-clock and database durations and reports execution statistics.
- Updates E2E configuration and CI logging for profiling runs.
File summaries
| File | Description |
|---|---|
xunit.runner.json |
Enables diagnostic test messages. |
test/Microsoft.Health.Fhir.Shared.Tests.E2E/xunit.runner.json |
Enables E2E diagnostics. |
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs |
Adds in-memory import coverage and metrics output. |
test/Configuration/testconfiguration.json |
Limits concurrent import test tasks. |
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs |
Measures database-call durations. |
src/Microsoft.Health.Fhir.SqlServer/Features/Operations/Import/SqlImporter.cs |
Aggregates database timing. |
src/Microsoft.Health.Fhir.SqlServer/Features/Operations/Import/ImportProcessingJob.cs |
Captures per-job elapsed time. |
src/Microsoft.Health.Fhir.SqlServer/Features/Operations/Import/ImportOrchestratorJob.cs |
Generates synthetic processing jobs. |
src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Operations/Import/ImportProcessingJobTests.cs |
Verifies timing propagation. |
src/Microsoft.Health.Fhir.Shared.Api/Features/Operations/Import/ImportRequestExtensions.cs |
Serializes the new request parameter. |
src/Microsoft.Health.Fhir.Shared.Api/Controllers/ImportController.cs |
Accepts in-memory import URLs. |
src/Microsoft.Health.Fhir.Core/Messages/BulkImport/CreateImportRequest.cs |
Carries synthetic-job configuration. |
src/Microsoft.Health.Fhir.Core/Features/Operations/IntegrationDataStoreClientConstants.cs |
Defines the reserved URI scheme. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/Models/ImportRequest.cs |
Extends the import request model. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/ImportProcessingProgress.cs |
Adds database timing progress. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/ImportProcessingJobResult.cs |
Adds per-job timing results. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/ImportOrchestratorJobDefinition.cs |
Persists fan-out configuration. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/ImportJobResult.cs |
Exposes execution statistics. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs |
Produces detailed and aggregate metrics. |
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/CreateImportRequestHandler.cs |
Propagates fan-out configuration. |
src/Microsoft.Health.Fhir.Core/Extensions/ImportMediatorExtensions.cs |
Extends import mediator arguments. |
src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/Import/GetImportRequestHandlerTests.cs |
Updates status-handler expectations. |
src/Microsoft.Health.Fhir.Azure/Microsoft.Health.Fhir.Azure.csproj |
Embeds the representative dataset. |
src/Microsoft.Health.Fhir.Azure/IntegrationDataStore/InMemoryTestDataSource.cs |
Implements the synthetic data source. |
src/Microsoft.Health.Fhir.Azure/IntegrationDataStore/AzureBlobIntegrationDataStoreClient.cs |
Routes reserved URLs in memory. |
src/Microsoft.Health.Fhir.Azure/IntegrationDataStore/TestData/representative-import-1000.ndjson.gz |
Supplies the profiling dataset. |
build/pr-variables.yml |
Increases PR deployment resources. |
build/jobs/e2e-tests.yml |
Prints execution statistics from TRX output. |
Review details
Suppressed comments (3)
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:170
- Applying
Take(100)before buildingjobResultsByIdalso truncates execution statistics. For example, polling the documented 1000-job import with_details=truereports and aggregates only 100 jobs; collect all job results for metrics and apply the detail limit only to response outcomes.
if (returnDetails)
{
completedJobs = completedJobs.Take(MaxDetailedJobs);
}
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:175
- Completed job rows can have a missing/null definition, result, or input location (the previous implementation explicitly skipped them). Direct deserialization now returns null and the following property access throws, making the entire status request fail; retain the defensive skip for incomplete persisted rows.
var definition = JsonConvert.DeserializeObject<ImportProcessingJobDefinition>(job.Definition);
var result = JsonConvert.DeserializeObject<ImportProcessingJobResult>(job.Result);
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:104
- Failed processing jobs are not guaranteed to retain an error result or input location; the prior code handled both cases. These direct dereferences/
new Uricalls can now throw while polling status instead of returning the intendedOperationFailedException, so restore null defaults and the no-error-file fallback.
var errorResult = JsonConvert.DeserializeObject<ImportJobErrorResult>(failed.Result);
var definition = JsonConvert.DeserializeObject<ImportProcessingJobDefinition>(failed.Definition);
if (errorResult.HttpStatusCode == 0)
{
errorResult.HttpStatusCode = HttpStatusCode.InternalServerError;
}
var resourceLocation = new Uri(definition.ResourceLocation);
- Files reviewed: 27/28 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Test fan-out is insufficiently gated, and current profiling can produce incomplete or storage-inflated results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Microsoft.Health.Fhir.Shared.Api/Controllers/ImportController.cs:221
- This test-only switch is enabled in every deployment for any caller with import permission, and the accepted upper bound creates up to 1,000,000 definitions in memory and submits them in one queue call. That can exhaust application/queue resources and pollute a production FHIR store with the embedded dataset. Gate this mode behind a disabled-by-default server setting (and use a deployment-appropriate fan-out limit) rather than trusting a request parameter alone.
if (importData.InMemoryTestProcessingJobs > 0
&& (importData.InMemoryTestProcessingJobs > 1_000_000
|| input.Count != 1
|| input[0].Url == null
|| !string.Equals(input[0].Url.Scheme, IntegrationDataStoreClientConstants.InMemoryTestSourceScheme, StringComparison.OrdinalIgnoreCase)))
{
throw new RequestNotValidException(string.Format(Resources.ImportRequestValueNotValid, nameof(importData.InMemoryTestProcessingJobs)));
- Files reviewed: 27/28 changed files
- Comments generated: 5
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The test endpoint permits unsafe fan-out, and the CPU aggregation can produce incomplete or inaccurate measurements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/Microsoft.Health.Fhir.Shared.Api/Controllers/ImportController.cs:216
- This test-only path is enabled on the normal production
$importendpoint with no dedicated feature/configuration gate, and the limit permits exactly 1,000,000 jobs. The orchestrator materializes that many inputs, definitions, and an array before enqueueing, then each job processes 1,000 resources; an import-authorized request can therefore exhaust service memory, flood the queue, and generate extreme database load. Gate this capability behind an explicit disabled-by-default test setting and enforce an operationally safe limit.
if (importData.InMemoryTestProcessingJobs > 0
&& (importData.InMemoryTestProcessingJobs > 1_000_000
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:172
- For an in-memory import with
returnDetails=true, this truncates completed jobs before failed outcomes and aggregate statistics are computed. Runs above 100 jobs will reportjobs=100, calculate CPU from only those jobs, and can hide per-resource failures in later completed jobs. Process every completed job for outcomes and aggregation; cap only the optional per-job lines emitted in the response.
if (returnDetails && suppressSuccessfulOutput)
{
completedJobs = completedJobs.Take(MaxDetailedJobs);
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:149
- The
F2values are formatted with the server's current culture, so this API field can emit values such as1,23instead of1.23and varies by deployment locale. Format the preformatted metrics invariantly so consumers and CI can parse them consistently.
var executionStats = new List<string> { $"jobs={jobLines.Count} cpu_msec_per_resource={cpuMillisecondsPerResource:F2} clock_msec={jobLines.Sum(_ => _.ClockMilliseconds)} database_msec={jobLines.Sum(_ => _.DatabaseMilliseconds)} retried_jobs={retriedJobs} parallelism={parallelism:F2}" };
- Files reviewed: 27/28 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The CPU estimate is affected by concurrent parsing and the 50-line result cap guarantees the new 60-job E2E test fails.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:123
- This subtraction does not isolate CPU time because resource loading/parsing runs concurrently on a separate
Taskwhile the importer awaits these timed SQL calls (ImportResourceLoader.cs:45-70andSqlImporter.cs:58-73). CPU spent parsing during a database call is subtracted, while scheduler/channel wait outside a database call is counted as CPU, socpu_msec_per_resourcevaries with overlap and does not measure the CPU consumption described by the PR. Use non-overlapping phases for this test mode or an actual CPU-time measurement.
var clockMilliseconds = _.Result.ClockMilliseconds;
var databaseMilliseconds = _.Result.DatabaseMilliseconds;
var cpuMilliseconds = clockMilliseconds - databaseMilliseconds; // x - null = null
src/Microsoft.Health.Fhir.Core/Features/Operations/Import/GetImportRequestHandler.cs:148
- The
F2values use the request thread's current culture, so the same API field can emit values such as1.25or1,25depending on deployment culture. Since these are preformatted metric records, format them invariantly so downstream collection/parsing is stable.
var executionStats = new List<string> { $"jobs={jobLines.Count} cpu_msec_per_resource={cpuMillisecondsPerResource:F2} clock_msec={jobLines.Sum(_ => _.ClockMilliseconds)} database_msec={jobLines.Sum(_ => _.DatabaseMilliseconds)} retried_jobs={retriedJobs} parallelism={parallelism:F2}" };
- Files reviewed: 32/33 changed files
- Comments generated: 2
- Review effort level: Balanced
Added ability to measure import CPU consumption based on walk-clock method:
Sample $import registration
POST {{fhir}}/$import
Content-Type: application/fhir+json
Prefer: respond-async
{
"resourceType": "Parameters",
"parameter": [
{ "name": "inputFormat", "valueString": "application/fhir+ndjson" },
{ "name": "input", "part": [ { "name": "url", "valueUri": "inmemorytest://whatever" } ] },
{ "name": "mode", "valueString": "IncrementalLoad" },
{ "name": "inMemoryTestProcessingJobs", "valueInteger": 1000 }
]
}