[confighttp] snappy compress, Encode and NewReader inconsistent behavior #10584

tomatopunk · 2024-07-10T10:00:54Z

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?


package main

import (
	"bytes"
	"fmt"
	"github.com/golang/snappy"
	"io"
)

func main() {
	test1()
	test2()
}

func test1() {
	data := []byte("Hello, World!")
	//encode
	compressed := snappy.Encode(nil, data)

	//decode
	reader := bytes.NewReader(compressed)
	sr := snappy.NewReader(reader)
	sb := new(bytes.Buffer)
	_, err := io.Copy(sb, sr)
	if err != nil {
		println("test1 decode error:", err.Error())
		return
	}

	fmt.Printf("test1 string1: %s, 2: %s", string(data), string(sb.Bytes()))
}

func test2() {
	data := []byte("Hello, World!")

	//encode
	var buf bytes.Buffer
	sw := snappy.NewBufferedWriter(&buf)
	_, err := sw.Write(data)
	if err != nil {
		println("encode error")
		return
	}

	//decode
	byteSlice := buf.Bytes()
	reqBuf, err := snappy.Decode(nil, byteSlice)
	if err != nil {
		println("test2 decode error:", err.Error())
		return
	}

	fmt.Printf("test1 string1: %s, 2: %s", string(data), string(reqBuf))

}

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:

opentelemetry-collector/config/confighttp/compression.go

Line 61 in 725e869

"snappy": func(body io.ReadCloser) (io.ReadCloser, error) {

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.

The text was updated successfully, but these errors were encountered:

BinaryFissionGames · 2024-07-12T15:11:05Z

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.

jpkrohling · 2024-07-15T07:21:31Z

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.

reader:

opentelemetry-collector/config/confighttp/compression.go

Lines 61 to 72 in 6fcebdb

    
           "snappy": func(body io.ReadCloser) (io.ReadCloser, error) { 
        
           	sr := snappy.NewReader(body) 
        
           	sb := new(bytes.Buffer) 
        
           	_, err := io.Copy(sb, sr) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	if err = body.Close(); err != nil { 
        
           		return nil, err 
        
           	} 
        
           	return io.NopCloser(sb), nil 
        
           },

writer:

opentelemetry-collector/config/confighttp/compressor.go

Lines 29 to 30 in 6fcebdb

    
           snappyPool                  = &compressor{pool: sync.Pool{New: func() any { return snappy.NewBufferedWriter(nil) }}} 
        
           _          writeCloserReset = (*zstd.Encoder)(nil)

tomatopunk · 2024-07-15T07:34:48Z

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.

tomatopunk · 2024-07-15T07:37:57Z

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

tomatopunk · 2024-07-15T07:43:53Z

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.

tomatopunk · 2024-07-15T07:53:50Z

so, I suggest either removing snappy decompress in the otlp-collector or replacing the current model with block format.

jpkrohling · 2024-07-15T08:03:03Z

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:

add "x-snappy-framed" working alongside "snappy" (v0.107.0)
deprecate "snappy" (v0.109.0)
remove "snappy" (framed) (v0.111.0)
add "snappy" (blocked) (v0.113.0)

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.

tomatopunk · 2024-07-15T08:12:32Z

Perhaps we could allow the receive component to choose whether to automatically decompress.

skandragon · 2024-11-27T17:21:40Z

This is a blocker for prometheus remote write, which I need to implement ASAP.

skandragon · 2025-04-11T05:44:56Z

This is still a blocker for the in-contrib prometheus-remote-write-receiver. It cannot function today.

tomatopunk · 2025-04-11T06:27:04Z

This is still a blocker for the in-contrib prometheus-remote-write-receiver. It cannot function today.

Hello @skandragon,

Yes, this is always a question. Perhaps you could send some emails to the code owners, like @codeboten, to get their opinions. The biggest challenge in solving this issue is finding a willing sponsor to support it.

If I were to implement it, I might choose to extend ServerConfig and modify the newBodyReader method. This could involve either forcibly disabling the Snappy decompression logic or providing a configuration option to specify the decompression type.

opentelemetry-collector/config/confighttp/compression.go

Lines 186 to 193 in a8f95d9

    
           func (d *decompressor) newBodyReader(r *http.Request) (io.ReadCloser, error) { 
        
           	encoding := r.Header.Get(headerContentEncoding) 
        
           	decoder, ok := d.decoders[encoding] 
        
           	if !ok { 
        
           		return nil, fmt.Errorf("unsupported %s: %s", headerContentEncoding, encoding) 
        
           	} 
        
           	return decoder(r.Body) 
        
           }

opentelemetry-collector/config/confighttp/confighttp.go

Line 414 in c43ecad

    
           func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settings component.TelemetrySettings, handler http.Handler, opts ...ToServerOption) (*http.Server, error) {

skandragon · 2025-04-11T07:02:30Z

@tomatopunk I went a different route by adding format detection logic to the confhttp package. Hopefully this will be able to make it in eventually, but until then, I have a branch up and working.

I do not think it is correct to keep using "snappy" content-type incorrectly, and this feels like it will unblock that concern, and ease migration to the correct wire format.

tomatopunk · 2025-04-11T08:49:38Z

@tomatopunk I went a different route by adding format detection logic to the confhttp package. Hopefully this will be able to make it in eventually, but until then, I have a branch up and working.

I do not think it is correct to keep using "snappy" content-type incorrectly, and this feels like it will unblock that concern, and ease migration to the correct wire format.

@skandragon , you are right. We should distinguish between the two types of Snappy instead of using the confusing snappy as the Content-Encoding field.

However, both Prometheus 1.0 and 2.0 protocols require Content-Encoding: snappy. Changing the protocol would take longer than adjusting the code for compatibility.

If you want to quickly resolve this issue, you should consider submitting a PR to modify the Prometheus remote write protocol.

https://prometheus.io/docs/specs/prw/remote_write_spec/#definitions

skandragon · 2025-04-11T09:58:26Z

@tomatopunk I think the issue is, content-encoding: snappy means, to everyone not the Otel collector, "block snappy". That is the real issue. Prometheus is not going to change, and I do not blame them as they are correct in how they have used that content header and the format of the data they sent.

In any case, the PR I raised seems to have gotten some attention. Let's see where that lands :)

#### Description This is an alternative PR to #12825. I'm taking all the commits from that PR and adding the feature gate on the client side, as requested by reviews. The server behavior of peeking into the initial bytes to identify the encoding is kept :) If you've reviewed the previous PR, you can just review the latest commit!  #### Link to tracking issue Fixes #10584 --------- Signed-off-by: Arthur Silva Sens <[email protected]> Co-authored-by: Michael Graff <[email protected]> Co-authored-by: Alex Boten <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]> Co-authored-by: Jonathan <[email protected]>

tomatopunk added the bug Something isn't working label Jul 10, 2024

tomatopunk changed the title ~~snappy compress, Encode and NewReader inconsistent behavior~~ [confighttp] snappy compress, Encode and NewReader inconsistent behavior Jul 12, 2024

jpkrohling self-assigned this Jul 12, 2024

jpkrohling closed this as completed Jul 15, 2024

jpkrohling reopened this Jul 15, 2024

skandragon mentioned this issue Apr 11, 2025

Proposed "snappy" vs "x-snappy-framed" content-type confusion #12825

Open

ArthurSens mentioned this issue Apr 22, 2025

Fix snappy/framed-snappy encoding/decoding #12911

Merged

mx-psi closed this as completed in #12911 May 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[confighttp] snappy compress, Encode and NewReader inconsistent behavior #10584

[confighttp] snappy compress, Encode and NewReader inconsistent behavior #10584

tomatopunk commented Jul 10, 2024 •

edited

Loading

BinaryFissionGames commented Jul 12, 2024 •

edited

Loading

jpkrohling commented Jul 15, 2024

tomatopunk commented Jul 15, 2024 •

edited

Loading

tomatopunk commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

jpkrohling commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

skandragon commented Nov 27, 2024

skandragon commented Apr 11, 2025

tomatopunk commented Apr 11, 2025

skandragon commented Apr 11, 2025

tomatopunk commented Apr 11, 2025

skandragon commented Apr 11, 2025 •

edited

Loading

[confighttp] snappy compress, Encode and NewReader inconsistent behavior #10584

[confighttp] snappy compress, Encode and NewReader inconsistent behavior #10584

Comments

tomatopunk commented Jul 10, 2024 • edited Loading

BinaryFissionGames commented Jul 12, 2024 • edited Loading

jpkrohling commented Jul 15, 2024

tomatopunk commented Jul 15, 2024 • edited Loading

tomatopunk commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

jpkrohling commented Jul 15, 2024

tomatopunk commented Jul 15, 2024

skandragon commented Nov 27, 2024

skandragon commented Apr 11, 2025

tomatopunk commented Apr 11, 2025

skandragon commented Apr 11, 2025

tomatopunk commented Apr 11, 2025

skandragon commented Apr 11, 2025 • edited Loading

tomatopunk commented Jul 10, 2024 •

edited

Loading

BinaryFissionGames commented Jul 12, 2024 •

edited

Loading

tomatopunk commented Jul 15, 2024 •

edited

Loading

skandragon commented Apr 11, 2025 •

edited

Loading