[Unified Recorder] Investigate ResetSanitizer

[HarshaNalluru]

#17379 added the ResetSanitizer but the feature doesn't seem to work as expected.
Investigate with @scbedd and find a way to fix it, update docs, add/unskip test.

[timovv]

I did a bit of digging here. During record mode, sanitizers are not applied until recording is stopped and we're writing the recording to disk. This means that if we do something like:

1. add a sanitizer
2. make a request
3. add ResetSanitizer (this removes the entry for the sanitizer added in (1))
4. make another request

Then the sanitizer will not be applied to either of the requests, since the sanitizers are only applied after all the requests have happened and the recording is complete. This means that ResetSanitizer doesn't actually do anything useful if placed in the middle of the test.

Our current ResetSanitizer test assumes that the above works, with the sanitizer only being applied to the first request.

In playback mode, the situation is different, with sanitizers being applied as each request comes in.

Is this a bug or working as intended? Have we ever actually come across a need for ResetSanitizer to be used in this way?

[scbedd]

@timovv This is a product of adding features without thinking holistically of the process.

1. Sanitizers apply at save time, because I had the thought in the back of my head about session vs request level Sanitizers. EG a sanitizer that applies, but requires the full context of the recording to properly apply. That would be implemented in our ContinuationSanitizer. Aside from that, we don't have any sanitizers that would require that full recording context IIRC.
2. ResetSanitizers route was added because necessity. Folks needed to be able to reset their sanitizers.
3. We never adjusted where sanitizers apply due to 1), but probably should have when we added ResetSanitizers.
4. To my knowledge, other than tests exercising usage of the test-proxy, no, I do not believe anyone changes sanitizers while a recording is ongoing. From the perspective of "need", I don't think this issue is super high unfortunately.

I have adjusted the naming of this issue to more properly reflect our discussion here.

EDIT: I will not rename this issue. I got here by notifications and didn't notice this was sdk-for-js. Will post new issue in sdk-tools.

[timovv]

Thanks for the follow up, Scott. Agreed that this is low priority. I'll update the title of this issue too and leave it in the backlog.

[timovv]

Actually, on second thought, I think that the new issue covers this one well. I'll close this one out.