Introduce a fhirpath debug tracer #3210

mmsmits · 2025-07-09T09:23:35Z

Description

Introduce a capability to inject a debug trace into the fhirpath evaluation.
This will be used by systems that want to trace execution and debug what happened at each stage.
(such as in the fhirpath-lab)

Related issues

Closes|Fixes|Resolves [issue #].

Testing

There is specific unit testing to verify the captuing of the trace informtion

FirelyTeam Checklist

Update the title of the PR to be succinct and less than 50 characters
Mark the PR with the label breaking change when this PR introduces breaking changes

…ant (that's the base class of identifier) Permits being able to differentiate in the expression tree between them.

only has minor impact on expression compilation, and no effect on runtime if no debugger injected.

Only inject the debugger break when in debug mode. This code should never be hit, so excluding the break is good. Intended to wake the developer if it ever hits here, as that means a new Expression type has been added that doesn't derived from one of the existing ones. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…e into the compiled expression

…variable processing

… show in the debug output what steps the evaluation took.

…ebugger

…al argument to the existing function. (from a usage side, looks the same)

brianpos · 2025-07-22T21:20:34Z

This isn't ready for merging just yet. I've discovered that context.GetThat() isn't the input collection, it's actually identical to GetThis() - so isn't helping with my debug tracing.
In fact I don't see the purpose of getThat() at all, value is ALWAYS the same as getThis()
Interface is good, injection needs review.

Made lots of updates, it should be right, but curious if there are any performance impacts.
The tree is unchanged without the debug injected, just some tracking of the focus is returned by the delgate and that's about it.
And the change that only impacts substring of all things.

WrapWithPropNullForFocus - refactored the code in Wrap into the function - this caused the focus to be evaluted twice

alexzautke · 2025-07-23T07:53:44Z

This isn't ready for merging just yet. I've discovered that context.GetThat() isn't the input collection, it's actually identical to GetThis() - so isn't helping with my debug tracing. In fact I don't see the purpose of getThat() at all, value is ALWAYS the same as getThis() Interface is good, injection needs review.

Made lots of updates, it should be right, but curious if there are any performance impacts. The tree is unchanged without the debug injected, just some tracking of the focus is returned by the delgate and that's about it. And the change that only impacts substring of all things.

Alright. Just let us know when you are ready.

brianpos · 2025-07-23T07:56:05Z

Alright. Just let us know when you are ready.

Sorry, I meant it's ready for you to review, though requesting you (or whoever) to be thorough, and happy to chat with whoever or do a call for a walk through.

Copilot

Pull Request Overview

This pull request introduces debug tracing capability for FHIRPath expression evaluation, allowing systems to inject debug tracers to monitor and trace execution steps at each stage of expression processing.

Adds IDebugTracer interface for capturing FHIRPath expression evaluation steps
Updates FhirPathCompiler to accept optional debug tracers during compilation
Implements comprehensive debug tracing throughout the expression evaluation pipeline

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/Hl7.Fhir.Base/FhirPath/IDebugTracer.cs`	Defines the debug tracer interface with TraceCall method
`src/Hl7.Fhir.Base/FhirPath/DiagnosticsDebugTracer.cs`	Provides default implementation using System.Diagnostics.Trace
`src/Hl7.Fhir.Base/FhirPath/FhirPathCompiler.cs`	Adds overloads accepting IDebugTracer parameter
`src/Hl7.Fhir.Base/FhirPath/Expressions/EvaluatorVisitor.cs`	Integrates debug tracing wrapper into expression evaluation
`src/Hl7.Fhir.Base/FhirPath/Expressions/Invokee.cs`	Updates delegate signature to support focus tracking for debug tracing
`src/Hl7.Fhir.Base/FhirPath/Parser/Grammar.cs`	Updates parser to use IdentifierExpression instead of ConstantExpression
`src/Hl7.FhirPath.R4.Tests/DebugTracerTests.cs`	Comprehensive unit tests validating debug tracer functionality

src/Hl7.Fhir.Base/FhirPath/DiagnosticsDebugTracer.cs

src/Hl7.Fhir.Base/FhirPath/Expressions/Invokee.cs

src/Hl7.FhirPath.R4.Tests/DebugTracerTests.cs

src/Hl7.Fhir.Base/FhirPath/Expressions/Invokee.cs

Kasdejong

I feel we are taking this in the wrong direction. This out parameter being ignored 9/10 times is bothering me. I feel like since our FP engine is built on immutability, saving this data on the fly should be a lot easier than it seems. After all, all invokees receive an old focus, and return a new one, but that does not make the old one any less valid. after the call, args[0] still contains the old focus. Or at least, it should.

Kasdejong · 2025-07-24T10:08:22Z

src/Hl7.Fhir.Base/FhirPath/DiagnosticsDebugTracer.cs

+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
+


We aim to have nullability annotations for any new classes. If those could be added, that would be great.

Added nullability.

Kasdejong · 2025-07-24T10:33:55Z

src/Hl7.Fhir.Base/FhirPath/DiagnosticsDebugTracer.cs

+            IEnumerable<KeyValuePair<string, IEnumerable<ITypedElement>>> variables)
+        {
+            string exprName;
+            if (expr is IdentifierExpression ie)


this chain of ifs could be refactored a BUNCH. maybe exprName = expr switch and then a single trace after using the result...

The traces aren't all the same, but pretty close.
Refactored the if else out though

Kasdejong · 2025-07-24T11:39:32Z

src/Hl7.Fhir.Base/FhirPath/Expressions/Invokee.cs

@@ -18,30 +18,49 @@

 namespace Hl7.FhirPath.Expressions;

-internal delegate FocusCollection Invokee(Closure context, IEnumerable<Invokee> arguments);
+internal delegate FocusCollection Invokee(Closure context, IEnumerable<Invokee> arguments, out FocusCollection focus);


this is a massive change. I don't see how this is necessary when the focus should be in arguments[0]. Can we not just save it in some outer scope? I see this out param being ignored very often, which is probably a sign it should not be there.

If you REALLY feel like you need it, we should overload this somehow.

The out parameter is only used by the debug tracer. Which is only injected by the visitor when configured.
Rest of the time it does nothing.

brianpos added 3 commits June 24, 2025 21:16

Functions and child properties the name is an identifier, not a const…

d24d890

…ant (that's the base class of identifier) Permits being able to differentiate in the expression tree between them.

ChildExpression name is an identifier (derived from constant)

269c6e3

Initial draft of injecting a debug tracer.

4d45c55

only has minor impact on expression compilation, and no effect on runtime if no debugger injected.

mmsmits mentioned this pull request Jul 9, 2025

FhirPath Engine function focus incorrect #3196

Open

Merge branch 'develop' into feature/BP-fhirpath-debugger

21539a4

mmsmits requested review from brianpos and Copilot July 17, 2025 08:11

This comment was marked as outdated.

Sign in to view

brianpos and others added 3 commits July 18, 2025 10:54

Complete the external surface for how to "inject" the tracing delegat…

cf9458b

…e into the compiled expression

Include the position information for the standard extension/valueset …

8cf189b

…variable processing

brianpos changed the title ~~Feature/bp fhirpath debugger~~ Introduce a fhirpath debug tracer Jul 18, 2025

brianpos added 4 commits July 18, 2025 17:45

Use an interface that has the function rather than a delegate.

bea44d0

rename debugtracer file

ba521f3

Include a unit test for the debug tracer

c0530a4

Tweak some other fhirpath unit tests to use the diagnostics tracer to…

a84bc97

… show in the debug output what steps the evaluation took.

brianpos mentioned this pull request Jul 18, 2025

FhirPath engine is 50% slower in later SDK5 versions #3174

Open

Merge remote-tracking branch 'hl7/develop' into feature/BP-fhirpath-d…

bdfc37f

…ebugger

brianpos marked this pull request as ready for review July 22, 2025 12:45

Change to a few overload to the function rather than adding an option…

7dd77e9

…al argument to the existing function. (from a usage side, looks the same)

brianpos added 2 commits July 23, 2025 14:16

Update the unit test with assertions that match the test data

e5754af

Capture the focus used in the invokee to report to the debug tracer

28ecc36

WrapWithPropNullForFocus - refactored the code in Wrap into the function - this caused the focus to be evaluted twice

mmsmits requested a review from Copilot July 23, 2025 07:58

Copilot AI reviewed Jul 23, 2025

View reviewed changes

mmsmits requested review from Kasdejong and removed request for brianpos July 23, 2025 08:30

Minor tweaks to cleanup co-pilots review

07c71d1

Merge branch 'develop' into feature/BP-fhirpath-debugger

29f0eeb

Kasdejong requested changes Jul 24, 2025

View reviewed changes

refactor if else into switches

81435f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce a fhirpath debug tracer #3210

Introduce a fhirpath debug tracer #3210

Uh oh!

mmsmits commented Jul 9, 2025 •

edited by brianpos

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

brianpos commented Jul 22, 2025 •

edited

Loading

Uh oh!

alexzautke commented Jul 23, 2025

Uh oh!

brianpos commented Jul 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kasdejong left a comment

Uh oh!

Kasdejong Jul 24, 2025

Uh oh!

brianpos Jul 24, 2025

Uh oh!

Kasdejong Jul 24, 2025

Uh oh!

brianpos Jul 24, 2025

Uh oh!

Kasdejong Jul 24, 2025

Uh oh!

brianpos Jul 24, 2025

Uh oh!

Uh oh!

Introduce a fhirpath debug tracer #3210

Are you sure you want to change the base?

Introduce a fhirpath debug tracer #3210

Uh oh!

Conversation

mmsmits commented Jul 9, 2025 • edited by brianpos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Testing

FirelyTeam Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

brianpos commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexzautke commented Jul 23, 2025

Uh oh!

brianpos commented Jul 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kasdejong left a comment

Choose a reason for hiding this comment

Uh oh!

Kasdejong Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

brianpos Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Kasdejong Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

brianpos Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Kasdejong Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

brianpos Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmsmits commented Jul 9, 2025 •

edited by brianpos

Loading

brianpos commented Jul 22, 2025 •

edited

Loading