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 intermediate transport layer between http/grpc client and exporter #809

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

Nevay
Copy link
Contributor

@Nevay Nevay commented Aug 29, 2022

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.

$transportFactory = new PsrTransportFactory(
    Psr18ClientDiscovery::find(),
    Psr17FactoryDiscovery::findRequestFactory(),
    Psr17FactoryDiscovery::findStreamFactory(),
);
$exporter = new Exporter($transportFactory->create('http://collector:4318/v1/traces'));
$transportFactory = new GrpcTransportFactory();
$exporter = new Exporter($transportFactory->create('http://collector:4317/opentelemetry.proto.collector.trace.v1.TraceService/Export'));

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #809 (bfa55b1) into main (ac3a35d) will decrease coverage by 0.46%.
The diff coverage is 68.33%.

Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
7.4 82.42% <68.33%> (-0.47%) ⬇️
8.0 82.45% <68.33%> (-0.47%) ⬇️
8.1 82.45% <68.33%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Contrib/OtlpHttp/MetricExporter.php 0.00% <0.00%> (ø)
src/SDK/Common/Export/Stream/StreamTransport.php 0.00% <0.00%> (ø)
...DK/Common/Export/Stream/StreamTransportFactory.php 0.00% <0.00%> (ø)
src/SDK/Common/Future/NullCancellation.php 0.00% <0.00%> (ø)
src/Contrib/Grpc/GrpcTransportFactory.php 53.48% <53.48%> (ø)
src/Contrib/Grpc/GrpcTransport.php 56.66% <56.66%> (ø)
src/Contrib/Otlp/StreamMetricExporter.php 57.14% <58.33%> (+18.01%) ⬆️
src/Contrib/OtlpHttp/Exporter.php 83.67% <82.22%> (-10.33%) ⬇️
src/SDK/Common/Export/Http/PsrUtils.php 89.36% <89.36%> (ø)
src/SDK/Common/Export/Http/PsrTransport.php 98.27% <98.27%> (ø)
... and 10 more

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 ac3a35d...bfa55b1. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Aug 30, 2022

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;
Copy link
Contributor Author

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.

Copy link
Collaborator

@brettmc brettmc left a 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...

src/Contrib/Grpc/GrpcTransportFactory.php Outdated Show resolved Hide resolved
src/SDK/Common/Export/Http/PsrTransport.php Show resolved Hide resolved
@Nevay Nevay requested a review from a team as a code owner September 7, 2022 19:57
@brettmc
Copy link
Collaborator

brettmc commented Sep 7, 2022

This looks like it replaces #677 ?

@brettmc brettmc linked an issue Sep 8, 2022 that may be closed by this pull request
@Nevay
Copy link
Contributor Author

Nevay commented Sep 9, 2022

It replaces it partly / resolves the original issue.
The generic RetryPolicyInterface approach can still be implemented on top of this PR (the grpc transport would need a rewrite to use an explicit retry loop similar to the http transport).

@bobstrecansky bobstrecansky merged commit c000590 into open-telemetry:main Sep 14, 2022
@Nevay Nevay deleted the feature/transport-layer branch April 19, 2023 19:11
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.

Handle partial success responses from OTLP export services
3 participants