Feature/injectable transport #116

Lewiscowles1986 · 2022-01-29T07:07:49Z

Fixes #115

Proposed code changes (minimal, interface based)
Test fix (I was authoring on Windows, and it was a pain, I could not run tests. It's 2-liner)
~~GitHub actions (until I'd fixed, I actually had a really slow feedback loop)~~
Editor config and git attributes
Proposed Documentation (words are hard, forgive me if it's not perfect).
Update to support PHP 8.0 & 8.1 thanks to Yoast
Tests
PHP 5.6 - PHP 8.1 (circleci missing 8.1-node-browsers image 🤷 )
Linting, Code-Style

The actual guard portion of this, doesn't have to be owned by this repo. In the README.md I added a section with example to bypass the magic, re-using the Ipv4Udp transport that is part of this PR, so that an example of instrumenting the control I was looking for (bypassing the magic in current constructor) is available.

I'm not 💯 sure about the construct with single method interface, but felt that opening and closing the socket separately to the functionality; could make some future testing easier. I Also didn't want to open a long-lived socket (although the classes don't hint at that, the way they are used in one place enforces for-now).

The interfaces are pretty stripped-back to deal with PHP 5.6 minimum. Not sure where DataDog is going with that, but I think users would get a lot of benefit rolling up to only 7.4 & 8+ in a future release.

Why interfaces

Short answer. I Hate the array approach. It's too open a design, and leads to choices where spooky [mis]configuration can happen. While this one interface won't change the entire codebase, it nudges in a direction a lot of PHP coders are going.

I'm not sure that adding strict_mode to counteract the magic will make this code any more readable or be the right way to counteract the magic cascade. For an upstream vendor to us this, implement the interface, I believe is a far more intentional choice, that leaves the majority of users happy where they are now. Internal getenv may be a feature for people who have not had silent misconfiguration lead to confusing bugs.

Another reason for an interface based approach is that I think a test implementation that allows spy test-double would be a nice thing that this allows. Instead of using curl spy test class and the socket spy test class, which are namespace coordination dependent.

Lastly, This interface approach would enable any coder:

To perform unit or integration level tests without needing to setup DataDog agent at all.
Could also help if falling-back in cases where perhaps the transport might use a PSR Logger, and send using Datadog API key as a log message.

gh123man · 2022-01-31T17:50:48Z

I like that this is a non-invasive change and should not break existing users.
Also thank you for the work in including PHP 8 support.

Could you add a more concrete of an example (or describe) as to why this approach is advantageous over checking the host/port configuration prior to constructing DogStatsd? Is there another transport type other than the UDP/UDS you are looking to support?

.github/workflows/ci.yml

src/TransportFactory.php

Minor: fopen('/dev/null', 'r') is not cross-platform. As much as this will run in a *nix environment. I like to run test-suites on ANY reasonable machine.

Thanks to Yoast!

Lewiscowles1986 · 2022-02-02T04:07:40Z

src/DogStatsd.php

-        $socket = is_null($this->socketPath) ? socket_create(AF_INET, SOCK_DGRAM, SOL_UDP)
-            : socket_create(AF_UNIX, SOCK_DGRAM, 0);
-        socket_set_nonblock($socket);
-
-        if (!is_null($this->socketPath)) {
-            $res = socket_sendto($socket, $message, strlen($message), 0, $this->socketPath);
-        } else {
-            $res = socket_sendto($socket, $message, strlen($message), 0, $this->host, $this->port);
-        }


IMO, this is a win.

Lewiscowles1986 · 2022-02-02T04:10:39Z

src/DogStatsd.php

+        $this->transportFactory = (
+            isset($config['transport_factory']) ?
+            $transportFactory :
+            new BridgingTransportFactory($this->socketPath, $this->host, $this->port)
+        );
+


What I had hoped might be pointed out is that this could be an alternative to https://github.com/DataDog/php-datadogstatsd/pull/116/files#diff-baf3cb376e36dfcf767f6389163de7d731798b1b108a44f33a2047e8e98bae3bR88-R98

I didn't want to leap too far in a single refactoring; and I think this should be part of a Configuration, that specific factory implementations could offer. So that implementations of transport are simple, and do not have cascading logic, or magic coming from the environment; but another layer can add that for them, or omit that, as the example in the README.md does.

gh123man

I think we are pretty close. Nice PR - and nice additions.
Mostly comments about the documentation and wording.

gh123man · 2022-02-09T20:38:40Z

src/DogStatsd.php

@@ -465,15 +477,7 @@ public function flush($message)
        $message .= $this->flushTelemetry();

        // Non - Blocking UDP I/O - Use IP Addresses!


NIT: This comment can be removed

Your comment?

yep - this comment is no longer relevant since the implementation moved (plus It's not a great comment to begin with)

gh123man · 2022-02-09T20:43:39Z

README.md

@@ -46,6 +46,64 @@ $statsd = new DogStatsd(

 DogStatsd constructor, takes a configuration array. See the full list of available [DogStatsD Client instantiation parameters](https://docs.datadoghq.com/developers/dogstatsd/?code-lang=php#client-instantiation-parameters).

+#### Migrating from legacy configuration


I'd call this section "Using a custom transport factory"

Overall I think this whole section is valuable but perhaps a bit verbose for the readme. Could you create a root level docs folder with this as a markdown document and link to it from the readme?

In this readme you could have a brief section

If you want more control over how transports are constructed see custom-transport-factory.md on how to implement a custom transport factory.

gh123man · 2022-02-09T20:48:40Z

README.md

+}
+```
+
+##### Instantiating datadog with your new class


Suggested change

##### Instantiating datadog with your new class

##### Instantiating DogStatsd with your custom transport factory

gh123man · 2022-02-09T20:51:27Z

README.md

+namespace YourOrg\Metrics;
+
+/**
+ * This class works around the decisions the constructor of DogStatsd class


NIT: reword to something like:

Custom transport factory that validates a host and port are not empty. This extends the default behavior where these errors would otherwise fail silently.

gh123man · 2022-02-09T20:53:42Z

README.md

@@ -46,6 +46,64 @@ $statsd = new DogStatsd(

 DogStatsd constructor, takes a configuration array. See the full list of available [DogStatsD Client instantiation parameters](https://docs.datadoghq.com/developers/dogstatsd/?code-lang=php#client-instantiation-parameters).

+#### Migrating from legacy configuration
+
+In some setups, the use of environment variables can cause surprises, such as warnings, test-failures, etc.


NIT: just want to reword this a bit. We prefer to be brief in the readme with a focus on usage. If you feel that the extended explanation is valuable, please add it in the code comments or in a dedicated document (as suggested above)

Perhaps something like (replacing lines 51-53):

Using environment variables for configuration may not always be ideal. If you want extra control over how transports are constructed or validated consider implementing a custom TransportFactory. You can achieve this by implanting the DataDog\TransportFactoryinterface and suppling it to thetransport_factory` configuration.

gh123man · 2022-02-09T20:57:51Z

README.md

+);
+```
+
+#### Why this structure?


I think we can drop this section from the readme. Perhaps add this as a code comment (or feel free to include it in a separate document)

gh123man · 2022-02-09T21:03:15Z

.circleci/config.yml

@@ -19,16 +19,28 @@ x-dockerbuild-5: &dockerbuild-5
    - run: composer lint

 jobs:
+  test-8.0:


Thanks for adding the extended test coverage 💯

gh123man · 2022-05-19T20:11:21Z

Ive cherrypicked the test fixes from this PR #127 and merged them.

gh123man requested changes Jan 31, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

src/TransportFactory.php Show resolved Hide resolved

Lewiscowles1986 force-pushed the feature/injectable-transport branch from d992eb3 to f9f4569 Compare February 2, 2022 03:08

Lewiscowles1986 added 5 commits February 2, 2022 03:20

Code changes

8b61f6e

Documentation

354de0e

Bonus: control line-endings

4fd3362

Fix tests on windows.

d4acf7c

Minor: fopen('/dev/null', 'r') is not cross-platform. As much as this will run in a *nix environment. I like to run test-suites on ANY reasonable machine.

PHP8 Compatibility

2a1c935

Thanks to Yoast!

Lewiscowles1986 force-pushed the feature/injectable-transport branch from f9f4569 to 2a1c935 Compare February 2, 2022 03:22

Lewiscowles1986 requested a review from gh123man February 2, 2022 03:24

Lewiscowles1986 commented Feb 2, 2022

View reviewed changes

gh123man reviewed Feb 9, 2022

View reviewed changes

gh123man force-pushed the master branch from bc69bfb to a361ae1 Compare January 3, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/injectable transport #116

Feature/injectable transport #116

Lewiscowles1986 commented Jan 29, 2022 •

edited

Loading

gh123man commented Jan 31, 2022

Lewiscowles1986 Feb 2, 2022

Lewiscowles1986 Feb 2, 2022

gh123man left a comment

gh123man Feb 9, 2022

Lewiscowles1986 Feb 11, 2022

gh123man Feb 11, 2022

gh123man Feb 9, 2022

gh123man Feb 9, 2022

gh123man Feb 9, 2022

gh123man Feb 9, 2022

gh123man Feb 9, 2022

gh123man Feb 9, 2022

gh123man commented May 19, 2022

		@@ -465,15 +477,7 @@ public function flush($message)
		$message .= $this->flushTelemetry();

		// Non - Blocking UDP I/O - Use IP Addresses!

		@@ -46,6 +46,64 @@ $statsd = new DogStatsd(

		DogStatsd constructor, takes a configuration array. See the full list of available [DogStatsD Client instantiation parameters](https://docs.datadoghq.com/developers/dogstatsd/?code-lang=php#client-instantiation-parameters).

		#### Migrating from legacy configuration

	##### Instantiating datadog with your new class
	##### Instantiating DogStatsd with your custom transport factory

Feature/injectable transport #116

Are you sure you want to change the base?

Feature/injectable transport #116

Conversation

Lewiscowles1986 commented Jan 29, 2022 • edited Loading

Why interfaces

gh123man commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh123man 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh123man commented May 19, 2022

Lewiscowles1986 commented Jan 29, 2022 •

edited

Loading