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

adding exporter + transport registry #862

Merged
merged 8 commits into from
Nov 23, 2022
Merged

adding exporter + transport registry #862

merged 8 commits into from
Nov 23, 2022

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Nov 16, 2022

To remove the hidden dependency between SDK and contrib, move all of the exporter knowledge out of exporter factory, and create a registry to hold those values.
The registry can be added to by contrib modules as part of composer autoloading. This also allows users to override things like transports (eg, provide their own grpc transport).
Remove exporter's fromConnectionString as it's not in spec, and didn't fit in with the registry idea.

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #862 (9ecdd81) into main (116e46d) will decrease coverage by 0.86%.
The diff coverage is 63.86%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #862      +/-   ##
============================================
- Coverage     80.67%   79.81%   -0.87%     
- Complexity     2011     2035      +24     
============================================
  Files           255      268      +13     
  Lines          5279     5271       -8     
============================================
- Hits           4259     4207      -52     
- Misses         1020     1064      +44     
Flag Coverage Δ
7.4 79.30% <63.86%> (-0.87%) ⬇️
8.0 79.80% <63.86%> (-0.87%) ⬇️
8.1 79.94% <63.86%> (-0.87%) ⬇️

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

Impacted Files Coverage Δ
src/Contrib/Jaeger/AgentExporter.php 100.00% <ø> (ø)
src/Contrib/Jaeger/HttpCollectorExporter.php 100.00% <ø> (+41.66%) ⬆️
src/Contrib/Newrelic/SpanExporterFactory.php 0.00% <0.00%> (ø)
src/Contrib/Otlp/MetricExporterFactory.php 0.00% <0.00%> (ø)
src/Contrib/Zipkin/SpanExporterFactory.php 0.00% <0.00%> (ø)
...Metrics/MetricExporter/InMemoryExporterFactory.php 0.00% <0.00%> (ø)
...trics/MetricExporter/NoopMetricExporterFactory.php 0.00% <0.00%> (ø)
src/SDK/SdkAutoloader.php 43.47% <0.00%> (ø)
src/SDK/Trace/Behavior/SpanExporterTrait.php 33.33% <ø> (-66.67%) ⬇️
.../Trace/SpanExporter/ConsoleSpanExporterFactory.php 0.00% <0.00%> (ø)
... and 68 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 116e46d...9ecdd81. Read the comment docs.

to remove the hidden dependency between SDK and contrib, move all of the exporter knowledge out of
exporter factory, and invent a registry to hold those values. The registry can be added to by contrib
modules as part of composer autoloading.
This also allows users to override things like transports (eg, provide their own grpc transport).
Remove exporter's fromConnectionString as it's not in spec, and I couldn't get it to work nicely with the
registry idea.
@brettmc
Copy link
Collaborator Author

brettmc commented Nov 16, 2022

I haven't yet tested for race conditions (eg between autloading and registering), file loading order. Also need to ensure sdk/contrib registered factories do not clobber user-defined ones.
update: because providers are lazy-loaded, we just need to ensure that user-provided factories are not clobbered by built-ins. I've added a clobber:bool parameter when registering factories to account for this.

adding an example of replacing a registered transport factory with another implementation
@brettmc brettmc marked this pull request as ready for review November 21, 2022 05:46
@brettmc brettmc requested a review from a team as a code owner November 21, 2022 05:46
src/SDK/Trace/SpanExporter/ConsoleSpanExporterFactory.php Outdated Show resolved Hide resolved
src/SDK/FactoryRegistry.php Outdated Show resolved Hide resolved
src/Contrib/Otlp/_register.php Outdated Show resolved Hide resolved
@bobstrecansky bobstrecansky merged commit 904da75 into open-telemetry:main Nov 23, 2022
@brettmc brettmc deleted the exporter-registry branch November 25, 2022 04:13
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.

None yet

3 participants