-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[dotnet] Add Execution Context #13978
base: trunk
Are you sure you want to change the base?
[dotnet] Add Execution Context #13978
Conversation
PR Description updated to latest commit (a6c82f6)
|
PR Review 🔍
|
PR Code Suggestions ✨
|
@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 |
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 |
….com/ChrstnAnsl/selenium into dotnet-fix-no-such-execution-context
Hi @pujagani updated the PR. Please see the changes. Thank you! |
LGTM. Thank you! @nvborisenko is the C# expert. Please help review it further. |
} | ||
|
||
using (HttpResponseMessage responseMessage = await this.client.SendAsync(requestMessage).ConfigureAwait(false)) | ||
{ | ||
if (_logger.IsEnabled(LogEventLevel.Trace)) | ||
{ | ||
_logger.Trace($"<< {responseMessage}"); | ||
|
||
if ((int)responseMessage.StatusCode < 200 || (int)responseMessage.StatusCode > 299) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not higher than 399? Do we care about logging redirects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @diemol, Great point. Please see the latest commit -- comment fixed. The updated value is now 399
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
….com/ChrstnAnsl/selenium into dotnet-fix-no-such-execution-context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, I really want to get to the bottom of #13920 !
Stop reviewing it please, this code even cannot be compiled. Does anybody know what is |
@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:
|
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
Checklist
PR Type
Bug fix, Enhancement
Description
HttpCommandExecutor
.Changes walkthrough 📝
HttpCommandExecutor.cs
Add detailed logging for HTTP request and response bodies
dotnet/src/webdriver/Remote/HttpCommandExecutor.cs