Implement OTLP/HTTP X-Protobuf Receiver #982
Conversation
kirbyquerby
requested review from
bogdandrutu,
dmitryax,
flands,
owais,
pjanotti,
rghetia,
songy23 and
tigrannajaryan
as code owners
receiver/otlpreceiver/otlp_test.go
Outdated
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Since this test largely replicates the test above that uses application/json (which is good since they're just testing different protocols), I'd refactor this so that both tests get the data from the same function/variable. And in general, maybe the tests could be refactored to share some other code as well.
There was a problem hiding this comment.
We tried to do something like this:
https://github.com/open-telemetry/opentelemetry-collector/blob/master/internal/data/testdata/trace.go
There was a problem hiding this comment.
Is this refactoring something I can do in this PR or should it be done in a separate PR?
receiver/otlpreceiver/otlp_test.go
Outdated
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
We tried to do something like this:
https://github.com/open-telemetry/opentelemetry-collector/blob/master/internal/data/testdata/trace.go
| @@ -80,7 +80,9 @@ func New( | |||
| r := &Receiver{ | |||
| ln: ln, | |||
| corsOrigins: []string{}, // Disable CORS by default. | |||
| gatewayMux: gatewayruntime.NewServeMux(), | |||
| gatewayMux: gatewayruntime.NewServeMux( | |||
| gatewayruntime.WithMarshalerOption("application/x-protobuf", &xProtobufMarshaler{}), | |||
There was a problem hiding this comment.
Should this be controlled by a config?
There was a problem hiding this comment.
I think it should be fine being on by default since it's just adding support for a new content-type to the existing HTTP server. When gzipped responses are added, though, maybe it could be useful to have the option to enable or disable them.
There was a problem hiding this comment.
I think it is fine to enable all transports in the receiver. I do not see why we would want to reject OTLP/HTTP incoming requests. However if we see that need for it we can add a config option in the future.
There was a problem hiding this comment.
I feel that defaults are fine but no option to disable, which may not be a problem now.
|
Can you please rebase? |
This change adds application/x-protobuf support to the existing grpc-gateway mux in the OTLP receiver. See: https://github.com/open-telemetry/oteps/blob/master/text/0099-otlp-http.md What currently works: * The receiver will correctly process HTTP requests with the Content-Type application/x-protobuf. * The receiver will respond with Content-Type application/x-protobuf. * The receiver will respond with status code HTTP 200 OK on success. What doesn't work yet: * The receiver cannot handle gzip-encoded requests (Content-Encoding gzip). * The receiver will not gzip-encode responses to requests with Accept-Encoding: gzip. Updates open-telemetry#881
runtime.ProtoMarshaller is now embedded in xProtoBufMarshaler so that only ContentType() needs to be implemented.
kirbyquerby
force-pushed
the
otlphttpxproto
branch
from
ca05475 to
6fb8ae4
Compare
Done. |
This change decreases the amount of duplicate code in the OTLP receiver tests. Context: open-telemetry#982 (comment)
This change decreases the amount of duplicate code in the OTLP receiver tests. Context: open-telemetry#982 (comment)
This change decreases the amount of duplicate code in the OTLP receiver tests. Context: open-telemetry#982 (comment)
This change decreases the amount of duplicate code in the OTLP receiver tests. Context: open-telemetry#982 (comment)
This change decreases the amount of duplicate code in the OTLP receiver tests. Context: open-telemetry#982 (comment)
|
@kirbyquerby can you amend the commit and push to force CircleCI build? |
kirbyquerby
force-pushed
the
otlphttpxproto
branch
from
6fb8ae4 to
f4e480d
Compare
|
@kirbyquerby I am not sure why but CircleCI does not want to build this PR. Can you close the PR and create a new one? |
|
It looks like CircleCI wasn't triggering because i had set it up on my own fork: |
* Set webhook port correctly * whoops
Description: This change adds application/x-protobuf support to the existing
grpc-gateway mux in the OTLP receiver.
Protocol spec: https://github.com/open-telemetry/oteps/blob/master/text/0099-otlp-http.md
What currently works:
Content-Type application/x-protobuf.
gzip) (http.Server natively supports gzip decoding).
What doesn't work yet:
Accept-Encoding: gzip.
Link to tracking Issue: #881
Testing: A test was added to verify that the receiver accepts application/x-protobuf requests.
Documentation: