[dotnet] Add Execution Context

[ChrstnAnsl]

User description

Description

Added Context to support and fix BUG #13920

Motivation and Context

I have encountered the same issue and wanting to debug it more but unfortunately there's not enough error logs to let me debug it

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Enhancement

---

Description

• Added detailed logging for HTTP request and response bodies in HttpCommandExecutor.
• Enhanced trace logging to include the content of HTTP requests and responses.
• Aimed to improve debugging capabilities by providing more comprehensive error logs.

---

Changes walkthrough 📝

	Relevant files
Enhancement	HttpCommandExecutor.cs Add detailed logging for HTTP request and response bodies dotnet/src/webdriver/Remote/HttpCommandExecutor.cs Added logging for request and response bodies. Enhanced trace logging to include request and response content. +8/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[CLAassistant]

All committers have signed the CLA.

[codiumai-pr-agent-pro]

PR Description updated to latest commit (a6c82f6)

• Copy walkthrough table to "Files Changed" Tab

[codiumai-pr-agent-pro]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to a single file and involve adding logging functionality which is straightforward. The logic is not complex, and the changes are well-contained.
🧪 Relevant tests	No
⚡ Possible issues	Performance Concern: The addition of logging for HTTP request and response bodies could potentially lead to performance degradation, especially if the content size is large. This might slow down the execution context significantly.
	Sensitive Data Exposure: Logging HTTP response and request bodies might inadvertently expose sensitive data in the logs, which could be a security risk if logs are not properly secured or if sensitive data is not adequately masked.
🔒 Security concerns	Sensitive Data Exposure: The new logging functionality could potentially expose sensitive information contained in HTTP requests or responses if not properly handled. This includes personal data, authentication tokens, or other sensitive information that might be part of the HTTP body.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a null check for responseMessage.Content before reading the response body Consider adding a null check for responseMessage.Content before attempting to read the response body to avoid potential NullReferenceException. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [301-302] -string responseBody = await responseMessage.Content.ReadAsStringAsync(); -_logger.Trace($"<< Body: {responseBody}"); +if (responseMessage.Content != null) +{ + string responseBody = await responseMessage.Content.ReadAsStringAsync(); + _logger.Trace($"<< Body: {responseBody}"); +} Suggestion importance[1-10]: 10 Why: This suggestion correctly identifies a potential NullReferenceException by adding a null check before accessing responseMessage.Content, which is crucial for robustness.	10
Performance	Use ConfigureAwait(false) when awaiting ReadAsStringAsync calls to avoid potential performance issues To avoid potential performance issues, consider using ConfigureAwait(false) when awaiting ReadAsStringAsync calls. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [291-292] -string content = await requestMessage.Content.ReadAsStringAsync(); +string content = await requestMessage.Content.ReadAsStringAsync().ConfigureAwait(false); _logger.Trace($">> Body: {content}"); Suggestion importance[1-10]: 8 Why: Using ConfigureAwait(false) is important in this context to prevent deadlocks and improve performance in asynchronous code. The suggestion is correct and targets the new code in the PR.	8
Maintainability	Extract the logging logic into a separate method for better readability and maintainability To improve readability and maintainability, consider extracting the logging logic into a separate method. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [286-294] -if (_logger.IsEnabled(LogEventLevel.Trace)) +LogRequestMessage(requestMessage); + +private async Task LogRequestMessage(HttpRequestMessage requestMessage) { - _logger.Trace($">> {requestMessage}"); - if (requestMessage.Content != null) + if (_logger.IsEnabled(LogEventLevel.Trace)) { - string content = await requestMessage.Content.ReadAsStringAsync(); - _logger.Trace($">> Body: {content}"); + _logger.Trace($">> {requestMessage}"); + if (requestMessage.Content != null) + { + string content = await requestMessage.Content.ReadAsStringAsync(); + _logger.Trace($">> Body: {content}"); + } } } Suggestion importance[1-10]: 7 Why: Extracting the logging logic into a separate method improves readability and maintainability, but it is not a critical change. The suggestion correctly identifies the relevant code block.	7
Best practice	Remove the unnecessary blank line at the end of the method Remove the unnecessary blank line at the end of the method to keep the code clean and consistent. dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [314-315] } - private Response CreateResponse(HttpResponseInfo responseInfo) Suggestion importance[1-10]: 4 Why: While removing unnecessary blank lines can improve code cleanliness, it is a minor stylistic improvement and not crucial to the functionality or readability of the code.	4

[nvborisenko]

@SeleniumHQ/selenium-committers should we include http request/response body of internal wire protocol to internal logs?

[ChrstnAnsl]

@SeleniumHQ/selenium-committers should we include http request/response body of internal wire protocol to internal logs?

Hi @nvborisenko I'll wait for everyone's opinion on whether we should include HTTP request/response bodies in internal logs. Let's gather everyone's thoughts about this then later on we can decide whether to include it or not. Thanks for looking up

[pujagani]

Hey! Thank you for your contribution. However, a typical concern with logging HTTP responses is the body size. For example, the response size might be huge when fetching the web page source. It might be a good idea to log the HTTP response body if the status code is not 2xx. Basically, logging in detail on error situations only.

[ChrstnAnsl]

Hey! Thank you for your contribution. However, a typical concern with logging HTTP responses is the body size. For example, the response size might be huge when fetching the web page source. It might be a good idea to log the HTTP response body if the status code is not 2xx. Basically, logging in detail on error situations only.

That's a great valid point. I will take it into consideration and update the PR accordingly. Thanks @pujagani

[ChrstnAnsl]

Hey! Thank you for your contribution. However, a typical concern with logging HTTP responses is the body size. For example, the response size might be huge when fetching the web page source. It might be a good idea to log the HTTP response body if the status code is not 2xx. Basically, logging in detail on error situations only.

That's a great valid point. I will take it into consideration and update the PR accordingly. Thanks @pujagani

Hey! Thank you for your contribution. However, a typical concern with logging HTTP responses is the body size. For example, the response size might be huge when fetching the web page source. It might be a good idea to log the HTTP response body if the status code is not 2xx. Basically, logging in detail on error situations only.

Hi @pujagani updated the PR. Please see the changes. Thank you!

[pujagani]

LGTM. Thank you! @nvborisenko is the C# expert. Please help review it further.

[diemol]

Why not higher than 399? Do we care about logging redirects?

[ChrstnAnsl]

Hi @diemol, Great point. Please see the latest commit -- comment fixed. The updated value is now 399

[nvborisenko]

The initial variant was correct, redirections are handled by lower HttpClientHandler, and are turned on by default (if I am not mistaken). Thus this code should never be executed in case of redirections.

[nvborisenko]

Wrote this comment and discovered another good option: if we want to log http requests/responses, we can do it in lower level... in HttpClientHandler. This way requires more code to be written (easy code), but it is more efficient. @ChrstnAnsl how do you feel comfortable in this topic?

[ChrstnAnsl]

Hi @nvborisenko I agree that logging HTTP requests/responses at the HttpClientHandler level does offer better efficiency. It's a good approach if we're looking to minimize overhead and streamline the logging process. If the extra code required for logging at the lower level isn't a concern and we prioritize performance, then implementing it in HttpClientHandler is definitely a solid option. Thanks for bringing it up!

Please see the latest updates. Thanks again!

[MJB222398]

Thanks for doing this, I really want to get to the bottom of #13920 !

[nvborisenko]

Stop reviewing it please, this code even cannot be compiled. Does anybody know what is IHttpInterceptor? When I mention http requests delegating I mean https://learn.microsoft.com/en-us/dotnet/api/system.net.http.delegatinghandler?view=net-8.0

[ChrstnAnsl]

@nvborisenko I see that there's some attributes and files are being ignored and not being pushed this is what's causing the error in the build. I will try another work around for my solution.

UPDATE:

• Apologies for the continuous build error now, I have updated my PR to handle https://learn.microsoft.com/en-us/dotnet/api/system.net.http.delegatinghandler?view=net-8.0. Thanks