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

Move gRPC driver to a subpackage and add an HTTP driver #1420

Merged
merged 19 commits into from
Jan 12, 2021

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Dec 22, 2020

This PR can be quite large, because I moved a bunch of code from one place to another. So maybe reviewing this PR will be more manageable if done commit-by-commit. If it's still too large then I suppose I can split the PR into three smaller ones like:

  • move gRPC driver to a subpackage
  • move some of the test code into a subpackage
  • add a new HTTP driver

Please let me know.

Anyway, the new HTTP driver supports only binary protobuf payloads. The support for JSON format would be a follow-up PR to this one.

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #1420 (6ac9ec7) into master (9332af1) will increase coverage by 0.1%.
The diff coverage is 82.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1420     +/-   ##
========================================
+ Coverage    78.6%   78.8%   +0.1%     
========================================
  Files         126     131      +5     
  Lines        6375    6753    +378     
========================================
+ Hits         5017    5326    +309     
- Misses       1114    1167     +53     
- Partials      244     260     +16     
Impacted Files Coverage Δ
exporters/otlp/options.go 100.0% <ø> (ø)
exporters/otlp/otlp.go 90.9% <ø> (ø)
exporters/otlp/internal/otlptest/data.go 61.7% <61.7%> (ø)
exporters/otlp/internal/otlptest/otlptest.go 74.5% <74.5%> (ø)
exporters/otlp/otlpgrpc/connection.go 85.9% <88.1%> (ø)
exporters/otlp/internal/otlptest/collector.go 88.8% <88.8%> (ø)
exporters/otlp/otlpgrpc/options.go 87.5% <90.0%> (ø)
exporters/otlp/otlphttp/driver.go 93.5% <93.5%> (ø)
exporters/otlp/otlpgrpc/driver.go 90.1% <100.0%> (ø)
exporters/otlp/otlphttp/options.go 100.0% <100.0%> (ø)
... and 5 more

exporters/otlp/otlphttp/driver.go Outdated Show resolved Hide resolved
exporters/otlp/otlphttp/driver.go Outdated Show resolved Hide resolved
fallthrough
case http.StatusServiceUnavailable:
select {
case <-time.After(getWaitDuration(d.cfg.backoff, i)):
Copy link
Member

Choose a reason for hiding this comment

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

Should this respect the Retry-After header in addition to performing exponential backoff?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it should, but I decided to punt that to some follow-up PR.

exporters/otlp/otlpgrpc/options.go Show resolved Hide resolved
exporters/otlp/otlphttp/driver.go Outdated Show resolved Hide resolved
exporters/otlp/otlphttp/driver.go Outdated Show resolved Hide resolved
exporters/otlp/otlphttp/driver.go Outdated Show resolved Hide resolved
exporters/otlp/otlphttp/options.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

I mostly skipped over the gRPC move, somewhat wishing the gRPC move and the new HTTP driver were in separate PRs, but wouldn't wish you to redo it either.

This is what the specification says for both gRPC and HTTP.
Currently it supports only binary protobuf payloads.
It also adds some common code mock collectors can use. This will be
useful for testing the HTTP driver.
It also extends the one record checkpointer a bit. This will be useful
for testing the HTTP driver.
We create our own instance of the transport, which is based on
golang's DefaultTransport. That way we sidestep the issue of the
DefaultTransport being modified/overwritten. We won't have any panics
at init. The cost of it is to keep the transport fields in sync with
DefaultTransport.
This may help with connection reuse.
@krnowak
Copy link
Member Author

krnowak commented Jan 8, 2021

Updated. Rebased to current master and fixed conflicts in changelog.

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.

6 participants