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

Implement OTLP/HTTP X-Protobuf Receiver #1021

Merged
merged 3 commits into from
May 26, 2020

Conversation

kirbyquerby
Copy link
Member

Description: This change adds application/x-protobuf support to the existing
grpc-gateway mux in the OTLP receiver.

(This change is a duplicate of #982 because CircleCI wasn't working.)

Protocol spec: 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.
  • The receiver can handle gzip-encoded requests (Content-Encoding
    gzip) (http.Server natively supports gzip decoding).

What doesn't work yet:

  • The receiver will not gzip-encode responses to requests with
    Accept-Encoding: gzip.

Link to tracking Issue: #881

Testing: A test was added to verify that the receiver accepts application/x-protobuf requests.

Documentation:

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.
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #1021 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
+ Coverage   85.61%   85.63%   +0.01%     
==========================================
  Files         189      190       +1     
  Lines       13167    13171       +4     
==========================================
+ Hits        11273    11279       +6     
+ Misses       1441     1440       -1     
+ Partials      453      452       -1     
Impacted Files Coverage Δ
receiver/otlpreceiver/otlp.go 86.72% <100.00%> (+0.23%) ⬆️
receiver/otlpreceiver/otlphttp.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 76.47% <0.00%> (+2.94%) ⬆️

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 daf2fc7...d3065a9. 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.

LGTM. Once OTLP/HTTP exporter is added we will need to add OTLP/HTTP testing to E2E tests.

@tigrannajaryan
Copy link
Member

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.

@kirbyquerby do you plan to submit a separate PR for these? Can you add a github issue so that we don't forget?

@tigrannajaryan tigrannajaryan merged commit 87af38a into open-telemetry:master May 26, 2020
@kirbyquerby
Copy link
Member Author

@tigrannajaryan for the first bullet point I was mistaken: Go's http server can already receive gzip-encoded requests (but still will not gzip responses). I am planning on submitting a PR for gzipped responses soon.

@flands flands added this to the Beta 0.4 milestone Jun 4, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
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
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
open-telemetry#1021)

* Bump github.com/golangci/golangci-lint from 1.29.0 to 1.30.0 in /tools

Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.29.0 to 1.30.0.
- [Release notes](https://github.com/golangci/golangci-lint/releases)
- [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md)
- [Commits](golangci/golangci-lint@v1.29.0...v1.30.0)

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

3 participants