Add php-amqplib integration #1996

PROFeNoM · 2023-03-31T15:10:36Z

Description

Add a php-amqplib integration (#1840).

Methods Traced

AMQPChannel

PhpAmqpLib\Channel\AMQPChannel::basic_get
PhpAmqpLib\Channel\AMQPChannel::basic_deliver
PhpAmqpLib\Channel\AMQPChannel::basic_publish
PhpAmqpLib\Channel\AMQPChannel::basic_cancel
PhpAmqpLib\Channel\AMQPChannel::basic_cancel_ok
PhpAmqpLib\Channel\AMQPChannel::basic_consume
PhpAmqpLib\Channel\AMQPChannel::basic_consume_ok
PhpAmqpLib\Channel\AMQPChannel::basic_ack
PhpAmqpLib\Channel\AMQPChannel::basic_nack
PhpAmqpLib\Channel\AMQPChannel::exchange_declare
PhpAmqpLib\Channel\AMQPChannel::queue_declare
PhpAmqpLib\Channel\AMQPChannel::queue_bind

AbstractConnection

PhpAmqpLib\Connection\AbstractConnection::connect
PhpAmqpLib\Connection\AbstractConnection::reconnect

Readiness checklist

(only for Members) Changelog has been added to the release document.
Tests added for this feature/bug.

Reviewer checklist

Appropriate labels assigned.
Milestone is set.
Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

pierotibou

Looking great, thanks. A quick review from my phone. I'll have another look on Monday

src/Integrations/Integrations/AMQP/AMQPIntegration.php

pierotibou · 2023-03-31T16:36:31Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+                $integration->setOptionalMessageTags($span, $message);
+
+                // Try to extract propagated context values from headers
+                $integration->extract($message);


extractContext may be a better name

pierotibou · 2023-03-31T16:37:40Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+                'prehook' => function (SpanData $span, $args) use ($integration) {
+                    /** @var AMQPMessage $message */
+                    $message = $args[0];
+                    $integration->inject($message);


injectContext

pierotibou · 2023-03-31T16:39:31Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+                    /** @var string $routing_key */
+                    $routingKey = $args[2] ?? '';
+
+                    $exchangeDisplayName = empty($exchange) ? '<default>' : $exchange;


Can we factorize duplicated code?

Note: I passed it locally, and I want to see if it will run fine on the CI as well

PROFeNoM · 2023-04-03T13:07:27Z

Ok, applied these changes. What's more, I also unmarked as 'skipped' the distributed tracing test so it can run on the CI (where it passes).

bwoebi · 2023-04-11T14:07:20Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+            // If present, set the traceid of the current span to x-datadog-trace-id
+            if (isset($headers['x-datadog-trace-id'])) {
+                $traceId = $headers['x-datadog-trace-id'];
+            } else {
+                return;
+            }
+
+            // If present, set the spanid of the current span to x-datadog-parent-id
+            if (isset($headers['x-datadog-parent-id'])) {
+                $parentId = $headers['x-datadog-parent-id'];
+            } else {
+                return;
+            }
+
+            // If present, set the sampling priority of the current span to x-datadog-sampling-priority
+            $priority = $headers['x-datadog-sampling-priority'] ?? null;
+            \DDTrace\set_priority_sampling($priority);
+
+            // If present, set the origin of the current span to x-datadog-origin
+            $origin = $headers['x-datadog-origin'] ?? null;
+
+            \DDTrace\set_distributed_tracing_context($traceId, $parentId, $origin);


Suggested change

// If present, set the traceid of the current span to x-datadog-trace-id

if (isset($headers['x-datadog-trace-id'])) {

$traceId = $headers['x-datadog-trace-id'];

} else {

return;

}

// If present, set the spanid of the current span to x-datadog-parent-id

if (isset($headers['x-datadog-parent-id'])) {

$parentId = $headers['x-datadog-parent-id'];

} else {

return;

}

// If present, set the sampling priority of the current span to x-datadog-sampling-priority

$priority = $headers['x-datadog-sampling-priority'] ?? null;

\DDTrace\set_priority_sampling($priority);

// If present, set the origin of the current span to x-datadog-origin

$origin = $headers['x-datadog-origin'] ?? null;

\DDTrace\set_distributed_tracing_context($traceId, $parentId, $origin);

\DDTrace\consume_distributed_tracing_headers($headers);

(Merge master into this first.)

bwoebi · 2023-04-11T14:09:24Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+        }
+    }
+
+    public function injectContext(AMQPMessage $message)


Add a check for DD_DISTRIBUTED_TRACING config first.

bwoebi · 2023-04-11T14:14:07Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+    public function setGenericTags(SpanData $span, string $name, string $spanKind, $exception = null)
+    {
+        $span->name = $name;


Suggested change

public function setGenericTags(SpanData $span, string $name, string $spanKind, $exception = null)

{

$span->name = $name;

public function setGenericTags(SpanData $span, string $name, string $spanKind, string $resourceDetail = null, $exception = null)

{

$span->name = "amqp.$name";

$span->resource = "$name" . ($resourceDetail === null ? "" : " $resourceDetail");

Wouldn't that reduce redundancy? Given that name and resource always share that part.

bwoebi · 2023-04-11T14:17:06Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+                    $exchangeDisplayName = empty($exchange) ? '<default>' : $exchange;
+                    $routingKeyDisplayName = empty($routingKey)
+                        ? '<all>'
+                        : (str_starts_with($routingKey, 'amq.gen-')
+                            ? '<generated>'
+                            : $routingKey);


Suggested change

$exchangeDisplayName = empty($exchange) ? '<default>' : $exchange;

$routingKeyDisplayName = empty($routingKey)

? '<all>'

: (str_starts_with($routingKey, 'amq.gen-')

? '<generated>'

: $routingKey);

$exchangeDisplayName = $this->formatExchangeName($exchange);

$routingKeyDisplayName = $this->formatRoutingKey($routingKey);

bwoebi · 2023-04-11T14:17:16Z

src/Integrations/Integrations/AMQP/AMQPIntegration.php

+            : (str_starts_with($routingKey, 'amq.gen-')
+                ? '<generated>'
+                : $routingKey);


Suggested change

: (str_starts_with($routingKey, 'amq.gen-')

? '<generated>'

: $routingKey);

: $this->formatQueueName($routingKey);

# Conflicts: # ext/ddtrace.stub.php

bwoebi · 2023-04-11T14:52:32Z

tests/Integrations/AMQP/AMQPTest.php

+    public function testDistributedTracing()
+    {
+        // Note: This test is extremely flaky, locally at least. It will eventually pass with some tries...
+        // Reason: We may parse the traces from dumped data BEFORE the traces are flushed.


Can you elaborate? Also \DDTrace\Tests\Common\TracerTestTrait::resetRequestDumper() is a thing, if needed.

For me it was never flaky, so, let's ship it.

PROFeNoM added 19 commits March 28, 2023 09:32

Setup files

f00bdf3

Start writing tests

5b869d5

Write the span assertions WITHOUT distributed tracing

d253725

Should add distributed tracing

fbd54ee

Write distributed test (FLAKY!)

89453b8

Remove fwrite

b860179

Fix class preloading issue

2965107

Fix linting issues

b201bdb

Fix api unit tests

87583e4

Trigger test run in the Makefile

73027b4

Change path

ffba7ac

Add rabbitmq image to ci

0059272

Remove 'ports' key

a2aa1c6

Fix a syntax issue

6e8c735

Use alpine rabbitmq image instead of management

0b0af1d

Syntax

f06ca8b

Change from management RMQ image to alpine

c285cde

Change integration loading procedure

076629b

Fix unit test

85120af

PROFeNoM requested a review from a team as a code owner March 31, 2023 15:10

PROFeNoM self-assigned this Mar 31, 2023

PROFeNoM added the 🎉 new-integration A new integration label Mar 31, 2023

pierotibou reviewed Mar 31, 2023

View reviewed changes

PROFeNoM added 3 commits April 3, 2023 14:25

Factorize display names & Add a safety check & Change fns names

d220f83

[TEST] Remove the distributed tracing test's skip

30a2d15

Note: I passed it locally, and I want to see if it will run fine on the CI as well

Remove outdated/debug comments

3dc89aa

Remove useless/redundant deferred loading method

177e04d

pierotibou approved these changes Apr 4, 2023

View reviewed changes

Add tests for phpamqp v2 for PHP7+

3fef74d

PROFeNoM added 4 commits April 7, 2023 09:54

Change vendor relative paths in test scripts

1dcad40

Remove deprecation warnings for the distributed tracing test

a3ea380

Remove deprecation warnings for the distributed tracing test

b15df6b

Remove amqp test for PHP7.0

5e80490

PROFeNoM added this to the 0.87.0 milestone Apr 11, 2023

bwoebi reviewed Apr 11, 2023

View reviewed changes

Merge branch 'master' into alex/integration/rabbitmq

7ae1278

# Conflicts: # ext/ddtrace.stub.php

bwoebi reviewed Apr 11, 2023

View reviewed changes

PROFeNoM added 3 commits April 11, 2023 17:41

[WIP?] Apply suggested changes

b69fdd4

Fix testBasicGet

11af6eb

Linting

a3e4c6c

bwoebi approved these changes Apr 17, 2023

View reviewed changes

PROFeNoM added the tracing label Apr 17, 2023

PROFeNoM merged commit 4504bbf into master Apr 21, 2023

PROFeNoM deleted the alex/integration/rabbitmq branch April 21, 2023 07:46

PROFeNoM mentioned this pull request Apr 21, 2023

Increase the number of configuration entries in zai config #2020

Merged

5 tasks

PROFeNoM mentioned this pull request May 12, 2023

Update PHP Tracer compatibilities DataDog/documentation#17610

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add php-amqplib integration #1996

Add php-amqplib integration #1996

PROFeNoM commented Mar 31, 2023

pierotibou left a comment

pierotibou Mar 31, 2023

pierotibou Mar 31, 2023

pierotibou Mar 31, 2023

PROFeNoM commented Apr 3, 2023 •

edited

bwoebi Apr 11, 2023 •

edited

bwoebi Apr 11, 2023

bwoebi Apr 11, 2023 •

edited

bwoebi Apr 11, 2023

bwoebi Apr 11, 2023

bwoebi Apr 11, 2023

bwoebi Apr 17, 2023

-    public function setGenericTags(SpanData $span, string $name, string $spanKind, $exception = null)
-    {
-        $span->name = $name;
+    public function setGenericTags(SpanData $span, string $name, string $spanKind, string $resourceDetail = null, $exception = null)
+    {
+        $span->name = "amqp.$name";
+        $span->resource = "$name" . ($resourceDetail === null ? "" : " $resourceDetail");

Add php-amqplib integration #1996

Add php-amqplib integration #1996

Conversation

PROFeNoM commented Mar 31, 2023

Description

Readiness checklist

Reviewer checklist

pierotibou 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

PROFeNoM commented Apr 3, 2023 • edited

bwoebi Apr 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoebi Apr 11, 2023 • edited

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

PROFeNoM commented Apr 3, 2023 •

edited

bwoebi Apr 11, 2023 •

edited

bwoebi Apr 11, 2023 •

edited