Skip to content

Improve exception logging and telemetry for hydration status#1912

Merged
tyrielv merged 2 commits intomicrosoft:masterfrom
tyrielv:tyrielv/hydration-status-1-itracer
Mar 18, 2026
Merged

Improve exception logging and telemetry for hydration status#1912
tyrielv merged 2 commits intomicrosoft:masterfrom
tyrielv:tyrielv/hydration-status-1-itracer

Conversation

@tyrielv
Copy link
Contributor

@tyrielv tyrielv commented Mar 17, 2026

  • Thread ITracer through EnlistmentHydrationSummary.CreateSummary()
  • Add Stopwatch timing per phase with HydrationSummaryDuration telemetry
  • Log RelatedWarning on early exit with specific invalid count values
  • Log RelatedError in catch block with elapsed duration
  • Add exception handling to LoadModifiedPaths file fallback with ITracer
  • Add null check in ReadModifiedPathDatabaseLines to prevent NRE

- Thread ITracer through EnlistmentHydrationSummary.CreateSummary()
- Add Stopwatch timing per phase with HydrationSummaryDuration telemetry
- Log RelatedWarning on early exit with specific invalid count values
- Log RelatedError in catch block with elapsed duration
- Add exception handling to LoadModifiedPaths file fallback with ITracer
- Add null check in ReadModifiedPathDatabaseLines to prevent NRE

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tyrielv tyrielv force-pushed the tyrielv/hydration-status-1-itracer branch from 8a751f1 to d695489 Compare March 17, 2026 15:41
Copy link

@KeithIsSleeping KeithIsSleeping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below

@KeithIsSleeping
Copy link

(Corrected re-post — the original review had rendering issues with backtick escaping)

Good changes overall — solid observability improvement. A few notes:

  1. Breaking signature change on CreateSummaryITracer tracer was added as a required (non-optional) parameter. If any callers exist outside this repo (e.g., in the hooks path or downstream consumers), they'll fail at compile time. Worth verifying there are no other callers. (PR Add CancellationToken and async hydration to GitStatusCache #1914 adds CancellationToken as optional with = default, but ITracer here is required.)

  2. Broadened pipe exception catchTryLoadModifiedPathsFromPipe changed from catch (BrokenPipeException) to catch (Exception). This now catches things like ArgumentException, InvalidOperationException, etc. that may indicate programming errors rather than pipe communication failures. Consider catch (IOException) or catch (BrokenPipeException) + catch (TimeoutException) for a tighter net, or at minimum log at Warning level (which you do — good) so these don't go unnoticed.

  3. Nit — The GetHeadTreeCount error messages use \nOutput: (single newline) and \n\nError: (double newline) inconsistently.

The per-phase Stopwatch timing with EmitDurationTelemetry is particularly well done — will make diagnosing slow phases trivial. The early-exit validation before the expensive GetHeadTreeCount is a smart optimization. The null check in ReadModifiedPathDatabaseLines fixes a real latent NRE.

Looks good to merge.

Resolve conflicts in EnlistmentHydrationSummary.cs and GitStatusCache.cs
by combining ITracer + Stopwatch timing from this branch with
CancellationToken support from PR microsoft#1914.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tyrielv tyrielv enabled auto-merge March 18, 2026 20:20
@tyrielv tyrielv merged commit 820341d into microsoft:master Mar 18, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants