-
Notifications
You must be signed in to change notification settings - Fork 203
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 intermediate transport layer between http/grpc client and exporter #809
Add intermediate transport layer between http/grpc client and exporter #809
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #809 +/- ##
============================================
- Coverage 82.88% 82.41% -0.47%
- Complexity 1829 1885 +56
============================================
Files 225 233 +8
Lines 4697 4869 +172
============================================
+ Hits 3893 4013 +120
- Misses 804 856 +52
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I've had a look through this PR, I really like the approach. My vote is to proceed. |
$this->setClient($client); | ||
$this->setRequestFactory($requestFactory); | ||
$this->setStreamFactory($streamFactory); | ||
$this->endpointUrl = $endpointUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: This is only for BC / to avoid extending the scope of this PR. I don't think that the current usage of $endpointUrl
as host
is correct (should be the service host and not the new relic endpoint?), host
and service.name
should likely be fetched from the span resource instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial review, looks great. I made a couple of observations...
- ignore invalid compression - add stream transport
3b0376f
to
bfa55b1
Compare
This looks like it replaces #677 ? |
It replaces it partly / resolves the original issue. |
Allows removing duplication between OtlpGrpc and OtlpHttp, both exporters can now use the same implementation with differing underlying transport. Also simplifies implementing async exporters by simply swapping the transport implementation as all of the relevant logic is encapsulated in the
TransportInterface
.Looking for feedback whether we want to go in this direction before adding tests - for now only tested with adapted
make smoke-test-collector-integration
.