Open tomatopunk opened 1 month ago
This mismatch between the reader/writer and encode/decode is called out in the library: https://pkg.go.dev/github.com/golang/snappy#pkg-overview - it explains there is a "stream" format and a "block" format.
Digging a little deeper, it looks like the stream/framed format should be specified as x-snappy-framed
, as described here: https://github.com/google/snappy/blob/main/framing_format.txt - I'd expect snappy
then to be exclusively for the block/non-framed format, like Prometheus remote write uses.
Thank you for digging into this, @BinaryFissionGames! I just double-checked, and we are using readers/writers consistently in the Collector code, I believe an OTel Collector as client can compress its data using snappy and a Collector server can read it. I'm closing this issue, but if I'm missing something, let me know and I'll reopen.
I believe this issue should be reopened.
Actually, we are discussing the need for consistency between streaming and framed encryption/decryption.
For example, stream encryption requires stream decryption, and framed encryption requires framed decryption. However, we are unsure about the correct methods to use for encryption and decryption.
origin:
Package snappy implements the Snappy compression format. It aims for very high speeds and reasonable compression.
There are actually two Snappy formats: block and stream. They are related, but different: trying to decompress block-compressed data as a Snappy stream will fail, and vice versa. The block format is the Decode and Encode functions and the stream format is the Reader and Writer types.
The block format, the more common case, is used when the complete size (the number of bytes) of the original data is known upfront, at the time compression starts. The stream format, also known as the framing format, is for when that isn't always true.
The canonical, C++ implementation is at https://github.com/google/snappy and it only implements the block format.
In the post, I mentioned to use of reader/write and encode/decode.
Specifically, if we use newBufferWrited
to encry and use decode
to decry, we will get an error snappy is incorrent
As I mentioned in this post, a classic example is the otlp-collector and prometheus remote write.
Prometheus remote write use snappy.Encode
for compress, while the otlp-collector use snappy.NewReader
. As a result, I encountered a snappy error, despite both using snappy.
so, I suggest either removing snappy decompress in the otlp-collector or replacing the current model with block format.
Looking at the documentation for that package, we are using what should be called "x-snappy-framed".
I'm not sure it's that easy for us to change from framed to block right now, as it would mean that clients and servers are not compatible anymore. I'd rather do a phased approach, where, once every couple of versions we take one of the following steps:
I believe the risk in breaking clients/servers this way is reduced, and we can then have another snappy support for block format, required by Prometheus remote write. Is it possible to implement framed format in Prometheus remote write? If so, we'd have it working on the next version already, due in a couple of weeks.
Perhaps we could allow the receive component to choose whether to automatically decompress.
Hello team, I hope you are having a good day.
Describe the bug
I am coding on the collect-contrib, about on a new component to receive prometheus_remote_write.
I have encountered a potential bug related to inconsistent behavior in Snappy compression when using encode -> NewReader and NewBufferedWriter -> decode.
When we compress or decompress from a stream, or read all of it and then compress or decompress it, we get different results
Steps to reproduce
What did you expect to see?
What did you see instead?
maybe should be correctly code/decode by snappy
What version did you use?
main branch
What config did you use?
code: https://github.com/open-telemetry/opentelemetry-collector/blob/725e8699bf43c71e1209dccbb444e480bee7bda7/config/confighttp/compression.go#L61
Environment
go 1.22
Additional context
In the case of Prometheus and OpenTelemetry collection, Prometheus uses encode and decode, while OpenTelemetry uses NewReader and NewBufferedWriter. However, I believe that whether you decode/encode in stream or read all and then encode/decode, the same protocol should yield the same results, and there should be no parsing failures.
code: https://github.com/prometheus/prometheus/blob/c173cd57c921f582586fc725ad51124728757533/cmd/promtool/metrics.go#L104
I would like to know how to address this bug. Should we consider skipping Snappy? If any fixes are required, I am willing to help resolve this issue.