Now we log the browser and the protocol version when attempting to li… #338
Conversation
| private async _getTargets(address: string, port: number): Promise<ITarget[]> { | ||
|
|
||
| // Get the browser and the protocol version | ||
| await this._getVersionData(address, port); |
There was a problem hiding this comment.
Can you have it only do this if /json/list fails? Or do you need this info all the time?
There was a problem hiding this comment.
What is your concern with doing this every time?
There was a problem hiding this comment.
I think this info will be helpful to us in all the cases irrespective of whether /list calls fails or not. Do you have any concerns?
There was a problem hiding this comment.
It's just doing extra work which will take a little time. How will it help when /list doesn't fail, you get the version info later too right?
There was a problem hiding this comment.
Yes but we don't get that info whenever there is a timeout which could be due to any other reasons as well and we do see a lot of customers having those.
There was a problem hiding this comment.
@roblourens What if we don't await this task, and we just do it in "background"? That way we are only spending CPU resources, and we are not making the user wait for this.
There was a problem hiding this comment.
Yes but we don't get that info whenever there is a timeout which could be due to any other reasons as well and we do see a lot of customers having those.
A timeout for /json/list? But you would be getting it from /version when /json/list fails.
But yeah, it would be ok if you don't await it. And make sure it doesn't break anything if the runtime doesn't support /json/list/version. Right now it looks like it will fail if that endpoint doesn't exist.
There was a problem hiding this comment.
I made it not to await and handled the fail scenario to just log the error message.
|
What's the scenario and use-case driving this? |
|
@auchenberg: It originated due to a request from the Edge team to help diagnose an issue. They are investigating failures where we sometimes do not hit the /json/list endpoint and the debug adapter times out due to it. They wanted to see if in those scenarios, we are able to hit the /json/version endpoint. We thought of adding this just for testing but then we realized that it may be useful data (to see the impact of this bug for customers and also to get the browser and the protocol version) in the product as well and we decided to make this change. Do you have any concerns over this? |
…st targets