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

[Core] Make RequestFailedDetailsParser easier to use for unbuffered streams #35355

Closed
annelo-msft opened this issue Apr 4, 2023 · 3 comments
Closed
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@annelo-msft
Copy link
Member

Today, any implementation of RequestFailedDetailsParser where the response content is an unbuffered network stream must overwrite response.ContentStream in order to log the "Content:" portion of the RequestFailedException message. Ideally, since any RequestFailedDetailsParser implementation must read the stream content to parse the exception, the interface would allow the caller to pass back the content it read and populate the "Content" on the message from that.

There are a couple other things that would be good to do here as well:

  • We could centralize the content buffering in Core when we have an error message, so each customer parser doesn't have to do this, potentially in a different way
  • We could either remove the message formatting in ClientDiagnostics entirely, in favor of the RFE implementation, or make these share common code -- currently this code is duplicated and different, so exception messages could be different depending on the code path that is followed.

Related to, and potentially could be done holistically as part of work on:

@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Apr 4, 2023
@annelo-msft
Copy link
Member Author

@AlexanderSher also made the good suggestion to consolidate buffering code to a single place in Core: #35318 (comment)

We should consider that as part of this work, and update ContainerRegistryRequestFailedDetailsParser.cs to use the common buffering code when it's available.

@AlexanderSher
Copy link
Contributor

If we assume that custom RequestFailedDetailsParser would like to parse the content, we can ensure the content stream is buffered (when content type header is present). That way, RequestFailedDetailsParser can directly call JsonDocument.Parse(response.ContentStream).

When RequestFailedDetailsParser is not specified, RequestFailedException.GetRequestFailedExceptionContent would call TryExtractErrorContent, which is not very effective (it creates readers and converts JSON input into UTF-16 string before converting it back). So using buffering code from ResponseBodyPolicy can help in the default case as well.

@JoshLove-msft
Copy link
Member

RequestFailedDetailsParser no longer operates on unbuffered content as of #35716

@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

3 participants