Fix tracking bug for jinja sampling

[nathaniel-may]

resolves #4038

Description

Comparison for tracking events should be done before we apply any changes. See ticket description for details.

Reviewers

I just reordered existing code so that the sample comparison is the first thing that's done in the function instead of the last.

Checklist

• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have updated the CHANGELOG.md and added information about my change

[jtcohen6]

The ordering here needs to be subtly different depending on the conditional branch we're following. In all cases, I'm using the reproduction case outlined in #4038.

1. If static parsing is enabled, and we're taking a stable sample: We need to compare jinja_sample_node + jinja_sample_config to node + config after we've updated the node's refs, sources, and config (i.e. after line 129). Otherwise, jinja_sample_node will include refs that our real node hasn't had the chance to add.
2. If we're taking an experimental parser sample (totally turned off for now): We can compare statically_parsed and experimental_sample anywhere along the way. But if we want to compare experimental_sample to the node itself, it needs to be before we've run update_parsed_node_config on that node, since that will add in refs from hooks. That's the problem in v0.21 right now.
3. If static parsing is disabled, or if the static/experimental parser doesn't return a dict, we're not taking any samples.

So I'm not sure that this reordering is the right move for main going forward, now that the static parser is on and actually mutating node by default. The biggest change we need to make is where we fire _get_exp_sample_result, both in main and in 0.21.latest.

If it simplifies the problem I'd be open to treating each branch separately, with separate PRs.

[nathaniel-may]

Chatted with @jtcohen6 and we figured out that we actually want to be doing this comparison on fully formed nodes. The experimental parser will likely be attempting to expand the capacity of the existing static parser so we cannot compare the two together naively. Instead will have to populate a complete copy of the node with everything after experimentally parsing and check if they match. Thankfully, this is a much sounder way to approach this comparison anyway.

[nathaniel-may]

Tried running the debugger with your project @jtcohen6 and I think something's still wrong. Let's connect on Monday.

[jtcohen6]

@nathaniel-may I just tried this out locally with the simple reproduction case, and it worked perfectly! The new approach also makes a lot of sense conceptually, and feels like something we could easily extend to other parsing methods (and even other node types).

It will be a bit tricky to backport this to 0.21.latest, given all the other changes we've made. As much as we can replicate the exact same approach over there—create an entire (deep)copy of the node, perform all mutations on it in isolation, then compare—I think that will serve us well to avoid spurious mismatches in v0.21.1.

[jtcohen6]

let's ship it! (see below)

[jtcohen6]

thanks for the detailed work teasing this one apart!