impl: improved logging and error collection for the http client #165
Conversation
For some clients, workspace polling fails due to the following error: ``` com.squareup.moshi.JsonEncodingException: Use JsonReader.setLenient(true) to accept malformed JSON at path $ ``` I haven’t been able to reproduce this issue, even using the same version deployed at the client (2.20.2). This change is an attempt to relax the JSON parsing rules by enabling lenient mode, in the hope that it will resolve the issue on the client side.
|
In theory it should decrease the risk but I can provide any guarantee until I am able to fully understand why the response is malformed/different. |
|
My initial impression is that this would only hide some other bug. Maybe a good first step would be to see if we can improve the error message? Like if we could log the request path and response. |
This is what I'm trying to figure out in between other priorities. Unfortunately by the time the exception is raised the response body has already been consumed by Moshi's converter, so you can't read it again. The interceptors are also not really a viable option IMHO, they are called before the converters which means:
Right now I'm playing with custom moshi converters and converter factories. From the docs it looks like it can be done, but I might have to manually write a lot of boilerplate hidden in moshi's factories. What are your thoughts on this @code-asher ? |
|
Would it be possible to use That is, assuming the interceptor works the way I hope it does 🤞 |
…I calls Wraps Moshi converter to log raw response body when JSON parsing fails. It helps debug malformed JSON during workspace polling by logging full response content, content type, and error details when a exception during marshalling occurs. A couple of approaches were tried, unfortunately by the time the exception is raised the response body has already been consumed by Moshi's converter, so you can't read it again. The interceptors are also not really a viable option, they are called before the converters which means: - we don't know if the response body is in the correct form or not. This is problematic because it means for every rest call we have to read the body twice (interceptor and by moshi converter) - we also have to save the intercepted body and store it until we have an exception from moshi, in which case it will have to be logged. This approach only logs on conversion failures, and the only performance impact on successful responses is the fact that we convert the response body byte stream to a string representation that can later be printed, and then again back to a byte stream by the moshi converter.
|
It works as you assume. But it means that when there is an issue the client will have to put Toolbox on Debug and reproduce the scenario again - and most of the time we have to go back to them and ask them to reproduce it in debug. This takes a couple of days of back and forth, and in some cases clients forget to ever get back with a proper reproduction :(. We can't log on info everything we intercept -cause that is too much so we need to log it on debug. Also, I'd like to point that the interceptor solution will do exactly the same thing (i.e. it has the same performance issues):
|
Yup that makes sense, but the cost is only paid when debugging is turned on, right?
Yeah this is unfortunate; but my thinking is we might have to ask them to do this anyway, since we might need to see the other requests to fully understand the issue (for example we have a couple issues in the VS Code plugin right now that have been impossible to reproduce or debug because we are not logging requests). But it is possible for this specific case just seeing the error will be enough to figure out the problem. Maybe it is reasonable to have both, log parsing errors on failure (what you have looks nice 👌), and have a debug toggle to log all requests if we need more information? |
That sounds great. |
|
Did a bit of research and it is unfortunate that JetBrains doesn't provide access to these settings so we will have to provide our own configuration. |
This commit adds support for allowing the user to configure the rest api client log level in the Settings page.
A new interceptor is now available in the rest client that is able to log different level of details regarding the request/response: - if None is configured by user we skip logging - Basic level prints the method + url + response code - Headers prints in addition the request and response headers sanitized first - Body also prints the request/response body
It is dangerous because it can allow dangerous operations in the plugin due to insufficient data that can be interpreted as default state.
For some clients, workspace polling fails due to the following error:
Although I’ve been unable to reproduce this issue — even using the exact version deployed at the client (2.20.2) — I've introduced a logging mechanism to improve diagnostics in such cases.
This PR introduces a configurable HTTP logging interceptor. Users can choose from various levels via the plugin UI:
Importantly, the logging converter remains in place to capture critical information during JSON deserialization failures, even when users have disabled detailed logging (e.g., to avoid logging full bodies).
To address the original error more effectively, I wrapped the Moshi converter with a custom Converter that logs the raw response body, content type, and exception details when a deserialization failure occurs. This helps debug malformed JSON responses, particularly during workspace polling.
This implementation only logs when deserialization fails. In the success path, the performance impact is minimal: the response body is converted to a string for potential logging, then re-wrapped as a stream for the Moshi converter. Users can opt in to always provide extra logging details but the constom converter ensures us that we have some minimum details regardless of user's choice.