Skip to content

Add intermediate transport layer between http/grpc client and exporter#809

Merged
bobstrecansky merged 7 commits into
open-telemetry:mainfrom
Nevay:feature/transport-layer
Sep 14, 2022
Merged

Add intermediate transport layer between http/grpc client and exporter#809
bobstrecansky merged 7 commits into
open-telemetry:mainfrom
Nevay:feature/transport-layer

Conversation

@Nevay

@Nevay Nevay commented Aug 29, 2022

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Aug 29, 2022 •
edited
Loading

Copy link
Copy Markdown

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

brettmc commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

I've had a look through this PR, I really like the approach. My vote is to proceed.

Nevay
Nevay commented Aug 30, 2022
View reviewed changes
Comment thread src/Contrib/Newrelic/Exporter.php
$this->setClient($client);
$this->setRequestFactory($requestFactory);
$this->setStreamFactory($streamFactory);
$this->endpointUrl = $endpointUrl;

@Nevay Nevay Aug 30, 2022

Copy link
Copy Markdown
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.

brettmc
brettmc reviewed Aug 31, 2022
View reviewed changes

@brettmc brettmc left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/Contrib/Grpc/GrpcTransportFactory.php Outdated
Comment thread src/SDK/Common/Export/Http/PsrTransport.php
Comment thread src/SDK/Common/Export/Http/PsrTransportFactory.php
@Nevay
Nevay force-pushed the feature/transport-layer branch from 3b0376f to bfa55b1 Compare September 7, 2022 19:57
@Nevay
Nevay requested a review from a team September 7, 2022 19:57
@brettmc

brettmc commented Sep 7, 2022

Copy link
Copy Markdown
Contributor

This looks like it replaces #677 ?

@brettmc brettmc linked an issue Sep 8, 2022 that may be closed by this pull request
Handle partial success responses from OTLP export services #802
Closed
@Nevay

Nevay commented Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

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

Reviewers

@brettmc brettmc brettmc approved these changes
@bobstrecansky bobstrecansky Awaiting requested review from bobstrecansky
@zsistla zsistla Awaiting requested review from zsistla
@tidal tidal Awaiting requested review from tidal

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle partial success responses from OTLP export services

3 participants

@Nevay @brettmc @bobstrecansky