Exporter factory, factory methods and creation could use some rework/cleanup

[tidal]

Is your request related to a problem?

At the moment the creation of span exporters (especially through the factory methods and the factory) class is inconsistent in behavior and in some scenarios broken. However in downstream instrumentations (for example Symfony Bundle) a generic way to instantiate exporters is needed to allow configuration.

1. ~~"fromConnectionString" factory methods in Jaeger, Newrelic, OtlpHttp, Zipkin and ZipkinToNewrelic exporters have a dependency on Guzzle library and will fail if other PSR7 implementations are used. see: 448~~
2. "fromConnectionString" factory method in OtlpGrpc and OtlpHttp exporters silently drop the passed "$endpointUrl" argument without using it and/or raising an error,
3. OtlpHttp exporter provides no way of configuring the endpoint (and a bunch of other options) programmatically, the only way is to provide a env var or using the default.
4. "fromConnectionString" signature in SpanExporterInterface defines a "$name" parameter which is not needed in all exporters . (However the "$name" parameter refers to the service name which is a required resource attribute
5. ExporterFactory requires a "$name" constructor argument, which is not required by all exporters (see above)
6. "fromConnectionString" method in SpanExporterInterface defines an untyped "$args" parameter, which suggests an array/list, but is only ever used a a string to pass the license key to Newrelic and ZipkinToNewrelic exporters
7. SpanExporterInterface requires all implementations to provide a "fromConnectionString" method, while future or custom implementations (for example for testing/debugging. see: 442) might not need a dsn/url at all. However ExporterFactory 's "fromConnectionString" requires a dsn/url to be provided.
8. ExporterFactory resolves the exporter to be created by an hard coded switch statement, which makes it hard to maintain. The only way to add custom exporters for users is to extend the factory class and override the entire "fromConnectionString" method. This violates Open/Closed Principle.
9. Exporter implementations have very different needs in terms of configuration. To provide an abstraction for the creation of exporters, there should be a way to instantiate an exporter from a set/array of options/parameters.
10. ~~The different exporter implementations have a lot of duplicate code.~~

Describe the solution you'd like

1. see: 448
2. Pass "$endpointUrl" argument to constructor
3. Make "$endpointUrl" an constructor parameter other options could be provided as an array in the constructor or get setters.
4. This is not too much of an issue, however maybe the "$name" paramter could be optional in the interface and/or the factory class could take the resource info as an optional(?) parameter and pull the service name from there (if given). The "fromConnectionString" could be put into a separate Interface which is only implemented by exporters which take/need the given parameters, this would also better respect Interface Segregation Principle.
5. See: 4.
6. The "$args" argument should be either an array or expect a http query string.
7. The Factory class should only require the contrib name to be given. It should be theresponsibilty of the exporter implementation to check and validate a DSN/URL (if needed).
8. The factory class should have some sort of registry or use a dedicated registry class for that purpose. This would make it easier to extend the list of exporter, especially for users of the SDK.
9. Exporters should provide a "fromOptions"/"fromArray" method or a dedicated factory, which takes care of that.
10. Duplicate code / behavior should be provided by traits, to make the exporters easier maintainable and avoid inheritance.

Describe alternatives you've considered

For the Symfony Bundle (and other instrumentations in the future) a consistent and reliable generic way to instantiate exporters is a must. I could solve most/all of above issues (which are relevant in this regard) in the Symfony Bundle (or opentelemetry-php-contrib). However I think it's better to tackle these issues in the SDK itself, than in downstream instrumentations.

Additional context

Since a lot of the issues above are sort of a blocker for me to finish the initial version of the Symfony Bundle, I would like to work on a fix. However before providing a PR, I would like to get some feeback and provide some examples of possible solutions.

What do you think about the issues I outlined or do you think some are no issues at all? What do you think about the possible solutions?

To not reinvent the wheel, Point 9. could be solved by leveraging the standalone Symfony OptionsResolver Component (or something similar). It provides a way for defining, validating, normalizing options and to provide defaults. However for the sake of the Dependency Inversion Principle I would be kind of hesitant to make the exporters directly depend on a third party library. So there should be an adapter and/or a dedicated factory which takes care of it.

I created a working example of how such factory could look like for the Newrelic exporter. (In real implementations a lot of duplicate code for the exporter factories could be extracted out into traits. See 10.): https://github.com/tidal/opentelemetry-php/blob/exporter_factory-options_resolver/src/Contrib/Newrelic/Factory.php

Because some (or all) exporters may not need a dedicated factory, I thought of a more generic kind of factory. This working example of e generic factory takes the fully qualified class name of an exporter as a constructor argument. It validates the class to implement SpanExporterInterface and configures the options by inspecting the class constructor via reflection.: https://github.com/tidal/opentelemetry-php/blob/exporter_factory-options_resolver/src/SDK/Trace/GenericExporterFactory.php

You can see usage examples for both factories in the code comments. I have no firm opinion on what might be a better solution if not both. They both have their pros and cons.

Those dedicated or generic factories could be used by the overall composite exporter factory to offload exporter creation to them. There is basically just some sort of mapping or registry needed (see 8.) In addition to or as a replacement of the "fromConnectionString" in the composite factory a "fromOptions"/"fromArray" method could be added.

[Grunet]

New to this repo, but I feel like I've noticed some of 2-6 as well recently.

Not being too familiar with things, I'd be curious how many decisions for things you've brought up would potentially need breaking changes to change them post-1.0 release (I couldn't quite figure that out from reading through). I assume those would possibly need more discussion than the rest? (there's also the CNCF Slack mentioned in the READme for closer to realtime discussions as well, though I'm not sure if that'd be needed here)

[tidal]

@Grunet I'm relaltively new to the repo as well, though I gave the package a try some months ago.

These are just some things which popped up while figuring out a way to provide configuration for the SDK in the Symfony Bundle and wiring up the individual parts, so I ent through a lit of possible scenarios to be at least aware of the.

Regarding breaking changes. The only thing that may break something would be changing the signature of the "fromConnectionString" methods. As far as I have seen neither these factory methods nor the factory itself are parts of the spec. Only the factory uses these methods as for now, and the sf bundle would be the first package to use the factory. As for now packagist only lists 2 packages depending on the SDK package. Ones seems to be not existing (URL leads to a 404), the other one is the Laravel package, which doesn't use the factory (methods).: https://packagist.org/packages/open-telemetry/opentelemetry/dependents?order_by=downloads

[SeanHood]

To add a small bit of context, regarding point 3. When I implemented OtlpGrpc, I continued having all the configurable options available in the constructor. Moving onto OtlpHttp, I figured there'd be less breaking changes to add this after the fact.

Since then, I proposed moving "common" items from OtlpGrpc and OtlpHttp into Otlp. Such as SpanConverter, and to bring this back around, a ConfigOpts object.

Ideas added to: https://github.com/open-telemetry/opentelemetry-php/issues/383

As I saw in #448, Discovery for PSR-17 and PSR-18 would also clean up these interfaces IMO.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[brettmc]

Closing, as I believe all of the issues originally identified have been resolved or are no longer relevant due to refactoring which has taken place over the past 12 months.