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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payload size isn't correct for some transactions #209

Closed
vbuberen opened this issue Jan 26, 2020 · 14 comments 路 Fixed by #391
Closed

Payload size isn't correct for some transactions #209

vbuberen opened this issue Jan 26, 2020 · 14 comments 路 Fixed by #391
Assignees

Comments

@vbuberen
Copy link
Collaborator

馃摙 Describe the bug

Some transactions show payload size as -1. It is totally fine, since OkHttp returns this value in case the size is unknown. However, when I wanted to handle this value in Chucker UI I decided to check what other network interceptors show and found out that Stetho show sizes for transactions, which show -1 in Chucker.

馃敡 Expected behavior

Transactions show actual size instead of -1.

馃摲 Screenshots

Screen Shot 2020-01-26 at 10 51 39

@vbuberen vbuberen added the bug Something isn't working label Jan 26, 2020
@vbuberen vbuberen added this to the 3.2.0 milestone Jan 26, 2020
@vbuberen vbuberen self-assigned this Jan 26, 2020
@vbuberen
Copy link
Collaborator Author

After some basic investigation found out that Chucker solely relies on contentLength value in response to show the size, while Stetho does some calculations itself in case response headers have no contentLength value. Requests from the screenshot in my bug report are exactly such items with no mentioned value in their response.

So Chucker needs to implement the same mechanism.

@MiSikora
Copy link
Contributor

MiSikora commented Mar 8, 2020

This could be fixed after merging #267. We would have to keep counting the bytes in TeeSource and report it back in the callback.

@MiSikora
Copy link
Contributor

MiSikora commented Mar 26, 2020

I was thinking about this feature lately and see couple of things that could be done.

  • If Content-Length is available Chucker could just use this value to determine the size of the response.
  • If Content-Length is not available Chucker could wait for the response to be read in a TeeSource and be informed about bytes read once upstream is depleted. The problem occurs if the consumer does not read bytes fully like reported in Chucker does not recognise partially read responses聽#287. In this case there is no way of knowing what would be the size of the response.
  • Chucker could display amount of data that was downloaded by the user and the size of the response if it is available (either form Content-Length or from TeeSource if exhausted). It would require some UI changes on most of the screens.
  • Chucker could show gradual changes of downloaded data. So while bytes are being read from an upstream it would display progressively 8 kB, 16 kB, 24 kB and so on. Not really sure if there is a need for this. It would also require some UI changes.

@vbuberen
Copy link
Collaborator Author

Moving further, so we could release 3.2.0

@MiSikora as to your ideas - agree on first 3 points, but the last one with progressive update of the request size is an overengineering, which won't bring much value to our users.

@MiSikora
Copy link
Contributor

Yes, I think so as well. Just wanted to propose it in case you find it valuable.

@cortinico
Copy link
Member

Agree on the first 3 points as well. On a side note I was also wondering if we should show the Content-Length aside the size.

I actually think it might be valuable for the user to see Content-Leght: -1 next to Size: xxx B to realise that the Content-Leght was missing.

@vbuberen
Copy link
Collaborator Author

We could show Unknown instead of -1 to be more specific or just drop the line with Content-Lenght at all from transaction description in such cases.

@cortinico
Copy link
Member

We could show Unknown instead of -1 to be more specific

++

@aldoKelvianto
Copy link

Hi, I would like to bump this issue because it still occurs on my project.

I've been using 3.1.2 and 3.2.0 release. It still produces incorrect content-size, while Chuck shows the correct content-size.

Chucker
Screenshot_1586144808

Chuck
Screenshot_1586145396

@vbuberen
Copy link
Collaborator Author

vbuberen commented Apr 6, 2020

Hi @aldoKelvianto
Thanks for reporting. The fix for this issue wasn't deployed yet, so there is nothing to bump :)

However, it is strange that Chuck shows the size while Chucker not.
Are these transactions have no Content-Length header?

@cortinico
Copy link
Member

Thanks for reporting @aldoKelvianto

It still produces incorrect content-size, while Chuck shows the correct content-size.

Can you please share the .addInterceptor(...) configuration you're using? Are you applying both Chuck and Chucker together? Do you have other interceptors? If yes, what's the order?

@aldoKelvianto
Copy link

However, it is strange that Chuck shows the size while Chucker not.
Are these transactions have no Content-Length header?

You guess it correctly. There's no Content-Length header in the API response. In Chucker's sample App, the responses contain content-length.

Screenshot_1586331382 copy

Screenshot_1586331376 copy

However, in my project, the response doesn't have content-length
Screenshot_1586345143 copy

However, it is strange that Chuck shows the size while Chucker not.

Yes, this surprises me as well 馃槃

@aldoKelvianto
Copy link

Thanks for reporting @aldoKelvianto
It still produces incorrect content-size, while Chuck shows the correct content-size.
Can you please share the .addInterceptor(...) configuration you're using? Are you applying both Chuck and Chucker together? Do you have other interceptors? If yes, what's the order?

You're welcome!

Sure, however, I don't think you'll find it interesting.

private fun createOkHttpClient(): OkHttpClient =
    OkHttpClient.Builder().apply {
        cache(createOkHttpCache())
        addInterceptor(ChuckerInterceptor(context))
        connectTimeout(CONNECT_TIMEOUT, TimeUnit.SECONDS)
        readTimeout(READ_TIMEOUT, TimeUnit.SECONDS)
    }.build()

I don't use other interceptors. I replace ChuckInterceptor with ChuckerInterceptor.

@cortinico
Copy link
Member

You guess it correctly. There's no Content-Length header in the API response.

Great. As discussed above we're probably going to decouple the Content-Length to the actual payload size in two different fields.

@vbuberen vbuberen added this to To do in Chucker board May 23, 2020
@ghost ghost added the Pending PR The resolution for the issue is in PR label Jul 19, 2020
Chucker board automation moved this from To do to Done and in Snapshot Jul 25, 2020
@ghost ghost removed Pending PR The resolution for the issue is in PR bug Something isn't working labels Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Chucker board
  
Done and in Snapshot
Development

Successfully merging a pull request may close this issue.

4 participants