Implement exporterfactory #4

alexperez52 · 2021-06-04T03:34:50Z

Description

This PR addresses issue 184. With this update, the exporter can be selected via configuration string. The implementation was done by creating a factory class called ExporterFactory that takes in a string through env variable (config string) and creates the correct Exporter. Prior to this change, exporters needed to be injected to the processors manually as instances.
Currently this update only support Jaeger and Zipkin exporters.

Type of change

New feature

Testing

Implemented Unit test that ensure that the exporter is being chosen dynamically based on env variable.
Testing verified through make test

Documentation

Documentation was added for the ExporterFactory file.

…only support Jaeger and Zipkin exporters

alexperez52 · 2021-06-04T12:17:51Z

Got some clarification that the factory should also handle the other exporters. Will update to reflect that

Aneurysm9 · 2021-06-04T17:29:00Z

sdk/Trace/ExporterFactory.php

+    public function fromConnectionString(string $configurationString)
+    {
+        $strArr = explode('+', $configurationString);
+        $contribName = $strArr[0];


This should be validated against an allowlist. This is untrusted user input and needs to be validated. Since we know the allowable values here an allowlist should be used to reject any non-allowed values. My PHP is incredibly rusty, but something like this should work:

private $_allowedExporters = array("Jaeger" => true, "Zipkin" => true); private function _isAllowed(string $exporter) { return array_key_exists($this->_allowedExporters, $exporter) && $this->_allowedExporters{$exporter}; } ... $contribName = ucfirst(lower($strArr[0])); if !$this->_isAllowed($contribName) { return null // or some way to indicate an error here } $dynamicExporter = ...

I agree with Aneurysm9, you must always sanitize any user input to protect from attacks. If we do have an allowed exporters list, then definitely use that as suggested.

Also look into the htmlentities() function which could serve as a way to sanitize input. Make sure it is relevant to your issue, usually it is used mostly to protect from front end malicious input.

Thanks for these reviews. Will soon have an update to reflect this

Aneurysm9 · 2021-06-04T17:30:06Z

tests/Sdk/Unit/Trace/ExporterFactoryTest.php

+    /**
+     * @test
+     */
+    public function testIfExporterHasCorrectEndpoint()


Be sure to add a test for invalid input as well. Perhaps even malicious input if you can construct one.

When writing you tests, make sure to use inputs that are malicious like certain html entities. There are examples out there that give examples of malicious attacks (especially with sql injections and other such attacks) although I don't know how relevant they will be to an exporter used for tracing.

ohamuy · 2021-06-04T18:49:56Z

sdk/Trace/ExporterFactory.php

+    public function fromConnectionString(string $configurationString)
+    {
+        $strArr = explode('+', $configurationString);
+        $contribName = $strArr[0];


I agree with Aneurysm9, you must always sanitize any user input to protect from attacks. If we do have an allowed exporters list, then definitely use that as suggested.

ohamuy · 2021-06-04T19:37:50Z

tests/Sdk/Unit/Trace/ExporterFactoryTest.php

+    /**
+     * @test
+     */
+    public function testIfExporterHasCorrectEndpoint()


When writing you tests, make sure to use inputs that are malicious like certain html entities. There are examples out there that give examples of malicious attacks (especially with sql injections and other such attacks) although I don't know how relevant they will be to an exporter used for tracing.

ohamuy · 2021-06-04T19:38:42Z

tests/Sdk/Unit/Trace/ExporterFactoryTest.php

+        $factory = new ExporterFactory('test.jaeger');
+        $exporter = $factory->fromConnectionString($input);
+        $this->assertInstanceOf(Path\Jaeger\Exporter::class, $exporter);
+    }


In terms of code structure these tests look good

ohamuy · 2021-06-04T19:42:08Z

sdk/Trace/ExporterFactory.php

+    public function fromConnectionString(string $configurationString)
+    {
+        $strArr = explode('+', $configurationString);
+        $contribName = $strArr[0];


Also look into the htmlentities() function which could serve as a way to sanitize input. Make sure it is relevant to your issue, usually it is used mostly to protect from front end malicious input.

…ort all exporters added

alexperez52 · 2021-06-05T00:54:06Z

Some of the generator functions seem redundant. I still wrote them in case of future changes in the implementation of the 6 different Exporters.

ohamuy

Everything else seems really good with good code coverage. Just the htmlentities mistake I told you about

ohamuy · 2021-06-06T17:12:55Z

sdk/Trace/ExporterFactory.php

+      */
+    public function fromConnectionString(string $configurationString)
+    {
+        $strArr = htmlentities($configurationString);


Ok so I made a mistake here, we should never escape input, its bad practice because it permanently ties that string to its final output format. So from what I understand you are usually only expecting some type of identifier with a url at the end. So what you want to do is filter the input to only be in the format you expect. Look at these filters here and here. You can also make your own filter through a regular expression which might be what you want. This will make sure that you are getting input only in the format that you desire.

From what I see, I don't believe these input values ever are on some type of html page so you can exclude the use of html entities. If it actually does end up on an html page you would use htmlentities() when your outputting it, not on input. So I think you can safely exclude htmlentities() since its just a url but definitely add the format filter you want instead

Used Nyholm Dsn parser for input string parsing. Modified unit tests and added testing for invalid licenseKey

sdk/Trace/ExporterFactory.php

Aneurysm9 · 2021-06-07T16:38:18Z

sdk/Trace/ExporterFactory.php

+        $licenseKey = '';
+        if ($endpointUrl != false) {
+            $dsn = DsnParser::parse($endpointUrl);
+            $endpointUrl = (string) ($dsn->withoutParameter('licenseKey'));
+            $licenseKey = (string) ($dsn->getParameter('licenseKey'));
+        }


This is overly-specific. The handling of licenseKey seems to only be applicable to the New Relic-flavored exporters. When new exporters are added that may have their own options this handling logic will need to change. Instead, if URL query string parameters are to be the mechanism for conveying configuration to exporters then they should be pulled into an array that is passed to the relevant constructor.

Aneurysm9 · 2021-06-07T16:41:23Z

sdk/Trace/ExporterFactory.php

+            return null;
+        }
+
+        // endpointUrl should only be null with otlp and otlpgrpc


If this is the case then it should be validated/enforced in some manner. Not really relevant to this PR, but out of curiosity how does the OTLP exporter get its target configuration? The exporter should definitely be able to be pointed at different destinations.

It seems that otlp- is constructed with no args and gets its params fron env variables

sdk/Trace/ExporterFactory.php

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

… into implement-exporterfactory

…11y/opentelemetry-php into implement-exporterfactory

kxyr · 2021-06-08T21:56:12Z

sdk/Trace/ExporterFactory.php

+        return $exporter;
+    }
+
+    private function _generateOtlp()


might this underscore have been forgotten to be removed?

yes. thank you good catch

kxyr · 2021-06-08T22:08:49Z

sdk/Trace/ExporterFactory.php

+        return array_key_exists($exporter, $this->allowedExporters) && $this->allowedExporters[$exporter];
+    }
+
+    // constructs the baseUrl from


(also another nit, but you may want to finish this comment)

…hed comment

alexperez52 added 4 commits June 3, 2021 11:49

Modified shouldSample documentation

2ce5a23

Implemented ExporterFactory with corresponding unit tests. Currently …

620d63b

…only support Jaeger and Zipkin exporters

Styling fixed for ExporterFactory and test file

e88e7b8

Modified code to remove psalm errors

f12a772

Aneurysm9 reviewed Jun 4, 2021

View reviewed changes

ohamuy reviewed Jun 4, 2021

View reviewed changes

Validation of user input added. Unit tests for validation added. Supp…

770eb81

…ort all exporters added

alexperez52 requested review from Aneurysm9 and ohamuy June 5, 2021 00:54

ohamuy reviewed Jun 6, 2021

View reviewed changes

Simplified exporter logic.

c08b03b

Used Nyholm Dsn parser for input string parsing. Modified unit tests and added testing for invalid licenseKey

Aneurysm9 reviewed Jun 7, 2021

View reviewed changes

License key and endpoint url are now generalized.

ace7093

Aneurysm9 approved these changes Jun 8, 2021

View reviewed changes

sdk/Trace/ExporterFactory.php Outdated Show resolved Hide resolved

alexperez52 and others added 3 commits June 8, 2021 16:34

Update sdk/Trace/ExporterFactory.php

bae03d8

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Merge branch 'master' of https://github.com/open-o11y/opentelemetry-php…

d300859

… into implement-exporterfactory

Merge branch 'implement-exporterfactory' of https://github.com/open-o…

dbd7412

…11y/opentelemetry-php into implement-exporterfactory

kxyr reviewed Jun 8, 2021

View reviewed changes

Removed underscore from otlp exporter functions and completed unfinis…

c9a94cc

…hed comment

alexperez52 merged commit 95c2dee into master Jun 9, 2021

ohamuy mentioned this pull request Jun 11, 2021

REQUEST: New membership for ohamuy open-telemetry/community#754

Closed

6 tasks

alexperez52 deleted the implement-exporterfactory branch June 18, 2021 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement exporterfactory #4

Implement exporterfactory #4

alexperez52 commented Jun 4, 2021

alexperez52 commented Jun 4, 2021

Aneurysm9 Jun 4, 2021

ohamuy Jun 4, 2021

ohamuy Jun 4, 2021

alexperez52 Jun 4, 2021

Aneurysm9 Jun 4, 2021

ohamuy Jun 4, 2021

ohamuy Jun 4, 2021

ohamuy Jun 4, 2021

ohamuy Jun 4, 2021

ohamuy Jun 4, 2021

alexperez52 commented Jun 5, 2021

ohamuy left a comment

ohamuy Jun 6, 2021

Aneurysm9 Jun 7, 2021

Aneurysm9 Jun 7, 2021

alexperez52 Jun 7, 2021

kxyr Jun 8, 2021

alexperez52 Jun 8, 2021

kxyr Jun 8, 2021

Implement exporterfactory #4

Implement exporterfactory #4

Conversation

alexperez52 commented Jun 4, 2021

Description

Type of change

Testing

Documentation

alexperez52 commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexperez52 commented Jun 5, 2021

ohamuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment