Querier error returned as compressed binary data

[leth]

If I run a certain query in graph mode, I am told: Error: rpc error: code = Code(413) desc = exceeded maximum number of samples in a query (100000)
If I run the same query in table mode, instead I am told: Server error (rpc error: code = Code(500) desc = ����������4�M �0�����V-���*ЕW��1��B��{ ��w��g3�<Śo�� U�����^)N�ܨٯ�-N��|�!�H̸���<�FDZ�n���H~[RO�=ݨ(w�Ou�a��xt�a:~��������������+W����� )

[leth]

This issue started after 6024681. @tomwilkie Perhaps this was introduced in #910 as it was the next build to release a new querier image?

[leth]

I'm not sure what fixed this, but it seems to no longer happen on the latest releases!

[leth]

Turns out we didn't test what we thought we had tested!

[leth]

Can confirm this is still an issue on the latest image

[bboreham]

It's working on 6024681c and broken on 57b71bbf, which is the first commit that produced a Querier image after the working one.

57b71bbf is from #910 which changed httpgrpc.pb.go.

[bboreham]

@leth gave me a tip which revealed what the corruption is - the code generating the error message is writing to a compressedResponseWriter, while ErrorFromHTTPResponse which pulls it out into gRPC is assuming it can use those bytes directly.

@tomwilkie does that raise any thoughts as to how we should avoid this?

[tomwilkie]

Can I look tomorrow? In Amsterdam for monitorama....

…

On Wed, 5 Sep 2018 at 19:17, Bryan Boreham ***@***.***> wrote: @leth <https://github.com/leth> gave me a tip which revealed what the corruption is - the code generating the error message is writing to a compressedResponseWriter, while ErrorFromHTTPResponse which pulls it out into gRPC is assuming it can use those bytes directly. @tomwilkie <https://github.com/tomwilkie> does that raise any thoughts as to how we should avoid this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#938 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbGhQOOIIRSdQ1LQmABFHvAbESwseL2ks5uYAcQgaJpZM4WD0aS> .

[bboreham]

So this was actually caused by #898 not #910, and the reason I was looking at the wrong commit is that a unit test failed on the merge for #910 https://circleci.com/gh/weaveworks/cortex/2700

[bboreham]

@tomwilkie yeah it can wait. Maybe I'll just delete httpgrpc 😉

[tomwilkie]

Meh, what did httpgrpc ever do for us?

…

On Wed, 5 Sep 2018 at 19:34, Bryan Boreham ***@***.***> wrote: @tomwilkie <https://github.com/tomwilkie> yeah it can wait. Maybe I'll just delete httpgrpc 😉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#938 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbGhaB49dUzsWw4s4EL4xEAH1rt44Maks5uYAsqgaJpZM4WD0aS> .

[bboreham]

One approach would be to inspect the content-encoding of the response buffer and decompress if necessary.

Seems like a lot of work.

[tomwilkie]

So this was actually caused by #898

I don't see how - would you mind explaining? AFAICT the Prometheus API Register has always compressed results. I don't see how it ever worked...

[tomwilkie]

One approach would be to inspect the content-encoding of the response buffer and decompress if necessary.

Not sure - everything should roundtrip. If we return a non-200 compressed response from the querier, it gets propagated as a grpc error from querier -> authfe, then converted back to a http response and written (compression header and all) back to the client.

[bboreham]

would you mind explaining

I haven't identified a specific code change, but we can make the issue come and go by switching to those querier versions noted above.

Even if round-trip was working, we'd still have a smaller problem that the error message is dumped into the OpenTracing span.

[bboreham]

I wonder if this relates to #780 which is also about corruption of response data.

[bboreham]

One difference between query and query_range is that the Prometheus API turns a storage error into an internal error (500), whereas it doesn't do that on query_range.

Cortex does send back storage errors, so we should avoid that in cases like exceeded maximum number of samples, but there is a deeper problem: that Prometheus switch statement actually treats all errors as storage errors, because a type assertion on an interface type is an 'implements' check, not an identical match.

Checking into where it breaks, I confirmed it returns 500s, but readable, 75de09a5 but returns compressed data in 57b71bbf, so it broke in #910. And if I update our front-end proxy to the same version of weaveworks/common the garbled data goes away, so I conclude the change to gRPC/proto generation was the culprit.

[bboreham]

Problems went away eventually after updating everything to the latest version after protobuf changes.