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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for gzip encoded payload in OTLP/HTTP receiver #1581

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

code-haven
Copy link
Contributor

Description:
Fixing a bug - OTLP/HTTP receiver does not support gzip. Fixed this by wrapping the HTTP handler generated by grpc-gateway to decompress prior to processing the request, similar to how it's done in

func processBodyIfNecessary(req *http.Request) io.Reader {

Link to tracking Issue:
#1344

Testing:
Modified unit tests to cover cases where request body is compressed via gzip/zlib.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 18, 2020

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #1581 into master will increase coverage by 0.00%.
The diff coverage is 92.40%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1581   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         261      262    +1     
  Lines       18540    18617   +77     
=======================================
+ Hits        16997    17068   +71     
- Misses       1113     1117    +4     
- Partials      430      432    +2     
Impacted Files Coverage Δ
receiver/otlpreceiver/otlphttp.go 78.57% <76.92%> (-21.43%) ⬇️
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
internal/middleware/compression.go 100.00% <100.00%> (ø)
receiver/otlpreceiver/otlp.go 95.00% <100.00%> (+0.19%) ⬆️
translator/internaldata/resource_to_oc.go 92.59% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 075828f...821c879. Read the comment docs.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@code-haven thank you for the PR, I left a couple comments/questions.

receiver/otlpreceiver/otlp.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/otlp.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@code-haven there are few unresolved comments, please ping when everything is resolved :)

@code-haven code-haven force-pushed the master branch 2 times, most recently from 846bee7 to 170780b Compare August 23, 2020 20:18
@bogdandrutu
Copy link
Member

Please rebase :)

@code-haven code-haven force-pushed the master branch 3 times, most recently from 0f9bf7f to 05ebf79 Compare August 25, 2020 14:41
config/confighttp/confighttp.go Outdated Show resolved Hide resolved
config/confighttp/confighttp.go Outdated Show resolved Hide resolved
@code-haven
Copy link
Contributor Author

code-haven commented Aug 26, 2020

@bogdandrutu @tigrannajaryan @jrcamp So I found a way to identify within the marshallers itself if the request has gzipped content by replicating what gzlib library does interally for validation.
I think this may be the right approach to solve this without resorting to the hacky approach of replacing the request body inside a middleware. Response compression can be handled via a middleware like https://github.com/nytimes/gziphandler.
I sincerely apologize for making so many changes in the PR :(. I'm converting this to a draft PR & also limiting scope to gzip only (as per the original issue). I should be ready with the final set of changes by tomorrow.

@code-haven code-haven marked this pull request as draft August 26, 2020 20:34
@code-haven code-haven changed the title Add support for gzip/zlib encoded payload in OTLP/HTTP receiver Add support for gzip encoded payload in OTLP/HTTP receiver Aug 26, 2020
@code-haven code-haven force-pushed the master branch 2 times, most recently from ac48f49 to be3d607 Compare August 26, 2020 22:08
@code-haven
Copy link
Contributor Author

Please rebase :)

@bogdandrutu I've decided to roll back the changes until this comment as I realize I'm complicating things by making changes to this PR without discussion. Sorry for all the confusion :(

Wanted to discuss if something like this would work instead of replacing the r.Body by peaking header bytes as defined in rfc1952? This would be implemented in xProtobufMarshaler

// NewDecoder returns a Decoder which reads proto stream from "reader".
func (marshaller *xProtobufMarshaler) NewDecoder(reader io.Reader) runtime.Decoder {
	return runtime.DecoderFunc(func(value interface{}) error {
		reader, gzipped, err := isGzip(reader)
		if err != nil {
			return err
		}
		if gzipped {
			gzReader, err := gzip.NewReader(reader)
			if err != nil {
				return err
			}
			reader = gzReader
			defer gzReader.Close()
		}
		buffer, err := ioutil.ReadAll(reader)
		if err != nil {
			return err
		}
		return marshaller.Unmarshal(buffer, value)
	})
}


func isGzip(input io.Reader) (io.Reader, bool, error) {
	const (
		gzipID1     = 0x1f
		gzipID2     = 0x8b
		gzipDeflate = 8
		peakLength  = 3
	)
        // standard gzip header bytes as defined in 
	reader := bufio.NewReader(input)
	headerBytes, err := reader.Peek(peakLength)
	if err != nil {
		return reader, false, err
	}
	isGzip := headerBytes[0] == gzipID1 && headerBytes[1] == gzipID2 && headerBytes[2] == gzipDeflate
	return reader, isGzip, nil
}

@code-haven code-haven force-pushed the master branch 6 times, most recently from 0d6ec22 to 46173e8 Compare September 4, 2020 18:03
@code-haven code-haven marked this pull request as ready for review September 4, 2020 18:03
receiver/otlpreceiver/otlphttp.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/otlphttp.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/otlphttp.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@code-haven please fix the build.

@code-haven code-haven force-pushed the master branch 3 times, most recently from 32aedb1 to 827cf24 Compare September 9, 2020 16:06
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with minor comment.

receiver/otlpreceiver/otlphttp.go Show resolved Hide resolved
receiver/otlpreceiver/otlp_test.go Show resolved Hide resolved
fallbackMsg := []byte(`{"code": 13, "message": "failed to marshal error message"}`)
fallbackContentType := "application/json"

switch statusCode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use if instead here.

@tigrannajaryan
Copy link
Member

@bogdandrutu do you have any other concerns or I can merge?

@bogdandrutu bogdandrutu merged commit 6c77fdb into open-telemetry:master Sep 15, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
…ry#1581)

* Fix validation for tracestate with vendor

* Add changes to changelog
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants