Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Test Proxy] Support matching GZip request content #4103

Closed
nisha-bhatia opened this issue Aug 31, 2022 · 14 comments · Fixed by #4165
Closed

[Test Proxy] Support matching GZip request content #4103

nisha-bhatia opened this issue Aug 31, 2022 · 14 comments · Fixed by #4165
Assignees
Labels
Test-Proxy Anything relating to test-proxy requests or issues.

Comments

@nisha-bhatia
Copy link
Member

To be completed by 2nd Preview in mid September
Explore if .Net gzip is deterministic?
When running tests in Record vs. Playback it seems like the length of the gzipped requests vary slightly. The test framework needs the a test's RequestBody's to match when running in Playback.
Possible solution: add a boolean property in TextProxy that for gzipped libraries that when true does not compare the Request Body in a Recording

@nisha-bhatia nisha-bhatia self-assigned this Aug 31, 2022
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 31, 2022
@azure-sdk
Copy link
Collaborator

Label prediction was below confidence level 0.6 for Model:ServiceLabels: 'Storage:0.20091417,Event Hubs:0.07821843,Search:0.068059646'

@nisha-bhatia nisha-bhatia removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 31, 2022
@JoshLove-msft
Copy link
Member

Test Proxy might be able to detect if request is Gzip based on content-encoding, and use that as a signal to decompress the bodies before comparing.

@jsquire jsquire added the Client This issue points to a problem in the data-plane of the library. label Sep 1, 2022
@pallavit
Copy link

This is an ask on the test framework. Gzip by nature is non-deterministic so we need a way to check if the content encoding is Gzip and compare the uncompressed request body instead in the proxy.
/cc:@nisha-bhatia Is this a blocker? Can we make the Gzip comparison tests live-only for preview and make this a GA criteria?

@JoshLove-msft
Copy link
Member

This should be handled in the Test Proxy itself, as it will be an issue for all languages.

@JoshLove-msft JoshLove-msft transferred this issue from Azure/azure-sdk-for-net Sep 12, 2022
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 12, 2022
@JoshLove-msft JoshLove-msft changed the title [Azure.Monitor.Ingestion] Gzip Testing [Test Proxy] Support matching GZip request content Sep 12, 2022
@nisha-bhatia
Copy link
Member Author

This is currently not a blocker because we have told the tests to not compare the text bodies of requests. It is something we should address soon though

@JoshLove-msft
Copy link
Member

/cc @scbedd

@jsquire
Copy link
Member

jsquire commented Sep 12, 2022

@JoshLove-msft: If this was handled in the test proxy, would that let Monitor just re-record the tests and things work - or would we need additional adjustments in the test framework to accommodate playback mode?

@JoshLove-msft
Copy link
Member

JoshLove-msft commented Sep 12, 2022

@JoshLove-msft: If this was handled in the test proxy, would that let Monitor just re-record the tests and things work - or would we need additional adjustments in the test framework to accommodate playback mode?

Ideally, the monitor tests would not need to be re-recorded. The Test Proxy should check the content-encoding of a request, and if it is Gzip, then it needs to decompress the request body and the recorded request body before comparing them.

@jsquire
Copy link
Member

jsquire commented Sep 12, 2022

Ah, gotcha. So the proxy does the comparison, not the framework. That's where I wasn't aware.

@JoshLove-msft
Copy link
Member

Ah, gotcha. So the proxy does the comparison, not the framework. That's where I wasn't aware.

Yep, the playback matching happens within the test proxy.

@scbedd
Copy link
Member

scbedd commented Sep 13, 2022

Can I please see what the requests would look like in json form?

We absolutely already automatically un-gzip request bodies when it is indicated by the header values. I suspect that the body differences are actually real.

@JoshLove-msft
Copy link
Member

JoshLove-msft commented Sep 13, 2022

Can I please see what the requests would look like in json form?

We absolutely already automatically un-gzip request bodies when it is indicated by the header values. I suspect that the body differences are actually real.

I think this is one example - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.Ingestion/tests/SessionRecords/MonitorIngestionLiveTest/ValidInputFromArrayAsJsonWithSingleBatchWithGzip.json#L17-L22

So the body is decompressed from the request when recording, but what about during playback? I don't see where the incoming request body is decompressed there. Without doing this, we end up comparing the compressed body with the decompressed one, so even without the issue of determinism (which may turn out to have not really been an issue here), the bodies won't match if the request is zipped.

@scbedd scbedd added Test-Proxy Anything relating to test-proxy requests or issues. and removed Client This issue points to a problem in the data-plane of the library. needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Sep 13, 2022
@scbedd scbedd self-assigned this Sep 13, 2022
@scbedd scbedd moved this to 🤔Triage in Azure SDK EngSys 🚢🎉 Sep 13, 2022
@scbedd
Copy link
Member

scbedd commented Sep 13, 2022

which may turn out to have not really been an issue here

I'm really thinking this is the case. We absolutely re-compress as we should, but it looks like there is a bug with decompressing the incoming. I'll add this to the list of "get these done ASAP" bugs for test-proxy.

The only other item on that list is for the further multipart/mixed fixes. I'm grinding super hard trying to get asset-sync stuff ready to go before MQ, but I'll definitely dedicate some time to this bug in the very near future.

@JoshLove-msft
Copy link
Member

JoshLove-msft commented Sep 18, 2022

I looked into this more and the proxy is storing decompressed Gzip for response bodies. This issue is specifically about the need to decompress request bodies when matching, and potentially when recording. If you look at the linked recording files, you can see that the data that was recorded contains a bunch of \uFFFD which is the unicode replacement character. This is because the binary Gzipped data is being translated to UTF-8 and stored as a string. So we will need to re-record these as the data is corrupted after the proxy is updated to handle storing Gzipped content (right now it assumes it is UTF-8 because the content type is JSON). Since we seem to be decompressing the response body when writing to the session files, it makes sense to do the same for request bodies.

Repository owner moved this from 🤔Triage to 🎊Closed in Azure SDK EngSys 🚢🎉 Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Test-Proxy Anything relating to test-proxy requests or issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants