Adds http transport by jcchavezs · Pull Request #9 · DataDog/dd-trace-php

jcchavezs · 2018-02-28T06:39:35Z

This PR includes the HTTP transport and a test against the actual agent.

kevinlebrun

Some nitpick but overall GTM.

kevinlebrun · 2018-03-06T13:35:56Z

+    {
+        $tracesPayload = $this->encoder->encodeTraces($traces);
+
+        $headers = $this->headers + ['Content-Type' => $this->encoder->getContentType()];


It is already done inside sendRequest

Good catch!

kevinlebrun · 2018-03-06T13:37:15Z

+                    sprintf('Reporting of spans failed, status code %d', $statusCode)
+                );
+            }
+        } else {


Doing this check in reverse can reduce the indentation level and make the code a little bit easier to read.

Good suggestion.

kevinlebrun · 2018-03-06T13:39:29Z

+                );
+            }
+        } else {
+            throw new RuntimeException(sprintf(


Do you plan to intercept this exception inside the tracer or do you expect the developer to catch this exception?

I prefer tracer to catch this and decide what to do with it: log it, increase metrics, etc.

Go returns the error https://github.com/DataDog/dd-trace-go/blob/master/tracer/transport.go#L108 and then send it over a channel: https://github.com/DataDog/dd-trace-go/blob/ba302ada363a5775d77da620ae9993f964b71f2a/tracer/tracer.go#L294 then logs the errors. Maybe we need an interface for ErrorHandler or something. @palazzem

To me it's fine till no exceptions must be handled by the user.

kevinlebrun · 2018-03-06T13:41:50Z

+        $tracer = new Tracer(new Noop);
+
+        $httpTransport = new Http(new Json, [
+            'endpoint' => 'http://0.0.0.0:8127/v0.3/traces'


Those tests assume you have an agent running. I wondering if this is a good idea to tag those tests with integration and let the possibility to run the complete test suite with and without external dependencies. Does it make sense?

Totally, I first thought to launch the agent with docker-php but it is meant for PHP 7.1. I like the tag thing but not sure how to do it (you mean annotations?)

Anyways, ideally it could be:
composer test: composer unit-test && composer integration-test
composer unit-test: phpunit run tests\Unit
composer integration-test: phpunit run tests\Integration

Makes sense?

Yes I was thinking about annotations. You can use @group (https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.group) for this purpose. Inside your phpunit.xml file you can then define test suites based on groups.

Since tests are separated in folders I think it is just easier to use that to separate the execution as described below:

Anyways, ideally it could be:
composer test: composer unit-test && composer integration-test
composer unit-test: phpunit run tests\Unit
composer integration-test: docker run ... && phpunit run tests\Integration && docker stop ...

What do you think?

Either way works for me.

kevinlebrun · 2018-03-06T13:44:54Z

      - checkout
+      - run:
+          name: Install php-bcmath
+          command: sudo docker-php-ext-install bcmath


This should be part of the README in a "requirements" section.

Totally what about postponing this for then it is time to write the README?

kevinlebrun · 2018-03-06T13:45:52Z


        if ($span->isFinished()) {
-            $arraySpan['duration_micro'] = 0;
+            $arraySpan['duration_micro'] = '-';


I prefer this way! It is clearer that a trick exists during the payload construction.

kevinlebrun · 2018-03-06T13:46:30Z

+     * @param string $hex
+     * @return string
+     */
+    public function hex2dec($hex)


I'm not sure this should be public.

It should be private.

kevinlebrun · 2018-03-06T13:47:34Z

+            curl_close($handle);
+
+            if ($statusCode === 415) {
+                throw new RuntimeException('Reporting of spans failed, try with version 0.2');


The message is misleading there are no implementation for version 0.2 right now, is it?

Right, what should we say instead? Upgrade your agent? cc @palazzem

kevinlebrun · 2018-03-09T09:44:10Z

+        "test-unit": "./vendor/bin/phpunit ./tests/Unit",
+        "test-integration":[
+            "echo \"Integration tests require the agent up and running. Use composer run-agent.\"",
+            "nc localhost 8126",


This directive will block if nothing is sent to nc. Did I miss something?

palazzem · 2018-03-09T10:16:48Z

+        curl_close($handle);
+
+        if ($statusCode === 415) {
+            throw new RuntimeException('Reporting of spans failed, try with version 0.2');


who catches these RuntimeExceptions? anyway the message should suggest to upgrade the client library to the latest versions. 415 is unsupported media type that means the client is sending something maybe correct to the wrong endpoint so it's the client that should upgrade, not the agent.

Regarding these exceptions I am not sure how DD wants to deal with them. In zipkin we inject a metrics service where we report when the the send fails. In my experience, logging this errors floods the logs. Any ideas? cc @palazzem @kevinlebrun

kevinlebrun

LGTM

jcchavezs added the ☠️ do-not-merge/WIP label Feb 28, 2018

jcchavezs force-pushed the adds_http_transport branch 4 times, most recently from a4726b1 to d27d4a2 Compare March 1, 2018 17:13

Adds HTTP transport.

f26964b

jcchavezs force-pushed the adds_http_transport branch from 89e6d4c to f26964b Compare March 5, 2018 07:32

jcchavezs removed the ☠️ do-not-merge/WIP label Mar 5, 2018

jcchavezs force-pushed the adds_http_transport branch from d7617bc to 6c720b9 Compare March 5, 2018 15:03

Adds support for bcmath.

65ba6df

jcchavezs force-pushed the adds_http_transport branch from 6c720b9 to 65ba6df Compare March 5, 2018 15:30

palazzem added the 🍏 core Changes to the core tracing functionality label Mar 6, 2018

palazzem added this to the 0.1.0 milestone Mar 6, 2018

palazzem requested review from kevinlebrun and vlad-mh March 6, 2018 13:22

kevinlebrun requested changes Mar 6, 2018

View reviewed changes

kevinlebrun requested changes Mar 9, 2018

View reviewed changes

palazzem reviewed Mar 9, 2018

View reviewed changes

jcchavezs force-pushed the adds_http_transport branch from bdd6bbe to 453e82c Compare March 10, 2018 13:59

Split test tasks into unit and integration.

c4da786

jcchavezs force-pushed the adds_http_transport branch 3 times, most recently from 95a7525 to cd3f4ec Compare March 12, 2018 17:25

Adds logger for HTTP transport.

1902dd4

jcchavezs force-pushed the adds_http_transport branch from cd3f4ec to 1902dd4 Compare March 12, 2018 17:40

Fixes tests for Http transport.

5d42770

jcchavezs force-pushed the adds_http_transport branch from 15bad5a to 5d42770 Compare March 14, 2018 07:02

kevinlebrun approved these changes Mar 14, 2018

View reviewed changes

jcchavezs merged commit fe401b5 into master Mar 14, 2018

jcchavezs deleted the adds_http_transport branch March 14, 2018 07:45

derickr mentioned this pull request Dec 7, 2023

[Bug]: Segmentation fault when running symfony/console help with Xdebug 3.3.0 enabled #2415

Closed

elzekool mentioned this pull request Apr 17, 2024

[Bug]: CLI PHP Processes do not stop and with Apache (mod_php) the connection is closed unexpectedly #2629

Open

morrisonlevi mentioned this pull request Jan 11, 2026

experiment: detect re-entrancy in ddtrace_serialize_closed_spans #3562

Draft

2 tasks

Conversation

jcchavezs commented Feb 28, 2018

Uh oh!

kevinlebrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcchavezs Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinlebrun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcchavezs Mar 6, 2018 •

edited

Loading