Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate eloquent integration to sandboxed API #559

Merged
merged 10 commits into from
Sep 18, 2019
3 changes: 2 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ aliases:
name: memcached_integration

- &IMAGE_DOCKER_MYSQL
image: mysql:5.6
image: docker.pkg.github.com/datadog/dd-trace-ci/php-mysql-dev:5.6
labbati marked this conversation as resolved.
Show resolved Hide resolved
name: mysql_integration
<<: *DD_TRACE_CI_DOCKER_CREDENTIALS
environment:
- MYSQL_ROOT_PASSWORD=test
- MYSQL_PASSWORD=test
Expand Down
1 change: 1 addition & 0 deletions bridge/dd_require_all.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
require __DIR__ . '/../src/DDTrace/Integrations/PDO/PDOIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/PDO/PDOSandboxedIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/Eloquent/EloquentIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/Eloquent/EloquentSandboxedIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/Memcached/MemcachedIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/Curl/CurlIntegration.php';
require __DIR__ . '/../src/DDTrace/Integrations/Mysqli/MysqliIntegration.php';
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@
"laravel-42-test": "@composer test -- tests/Integrations/Laravel/V4",

"laravel-57-update": "@composer --working-dir=tests/Frameworks/Laravel/Version_5_7 update",
"laravel-57-test": "@composer test -- tests/Integrations/Laravel/V5_7/CommonScenariosTest.php",
"laravel-57-test": "@composer test -- tests/Integrations/Laravel/V5_7",

"laravel-58-update": "@composer --working-dir=tests/Frameworks/Laravel/Version_5_8 update",
"laravel-58-test": "@composer test -- --testsuite=laravel-58-test",
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ services:
'fpm': { <<: *base_php_service, image: 'circleci/ruby:2.5', depends_on: [] }

mysql_integration:
image: mysql:5.6
image: docker.pkg.github.com/datadog/dd-trace-ci/php-mysql-dev:5.6
ports:
- "3306:3306"
environment:
Expand Down
2 changes: 1 addition & 1 deletion phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<directory>tests/Integrations/CLI/CakePHP/V2_8</directory>
</testsuite>
<testsuite name="laravel-58-test">
<file>tests/Integrations/Laravel/V5_8/CommonScenariosTest.php</file>
<directory>tests/Integrations/Laravel/V5_8</directory>
<directory>tests/Integrations/CLI/Laravel/V5_8</directory>
</testsuite>
<testsuite name="slim-312-test">
Expand Down
13 changes: 5 additions & 8 deletions src/DDTrace/Integrations/Eloquent/EloquentIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,9 @@ public static function load()
return dd_trace_forward_call();
}

list($eloquentQueryBuilder) = func_get_args();
$scope = $tracer->startIntegrationScopeAndSpan($integration, 'eloquent.insert');
$span = $scope->getSpan();
$sql = $eloquentQueryBuilder->getQuery()->toSql();
$span->setTag(Tag::RESOURCE_NAME, $sql);
$span->setTag(Tag::DB_STATEMENT, $sql);
$span->setTag(Tag::RESOURCE_NAME, get_class($this));
$span->setTag(Tag::SPAN_TYPE, Type::SQL);

return include __DIR__ . '/../../try_catch_finally.php';
Expand All @@ -84,9 +81,7 @@ public static function load()

$scope = $tracer->startIntegrationScopeAndSpan($integration, 'eloquent.update');
$span = $scope->getSpan();
$sql = $eloquentQueryBuilder->getQuery()->toSql();
$span->setTag(Tag::RESOURCE_NAME, $sql);
$span->setTag(Tag::DB_STATEMENT, $sql);
$span->setTag(Tag::RESOURCE_NAME, get_class($this));
$span->setTag(Tag::SPAN_TYPE, Type::SQL);

return include __DIR__ . '/../../try_catch_finally.php';
Expand All @@ -100,7 +95,9 @@ public static function load()
}

$scope = $tracer->startIntegrationScopeAndSpan($integration, 'eloquent.delete');
$scope->getSpan()->setTag(Tag::SPAN_TYPE, Type::SQL);
$span = $scope->getSpan();
$span->setTag(Tag::SPAN_TYPE, Type::SQL);
$span->setTag(Tag::RESOURCE_NAME, get_class($this));

return include __DIR__ . '/../../try_catch_finally.php';
});
Expand Down
130 changes: 130 additions & 0 deletions src/DDTrace/Integrations/Eloquent/EloquentSandboxedIntegration.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
<?php

namespace DDTrace\Integrations\Eloquent;

use DDTrace\Configuration;
use DDTrace\Integrations\Integration;
use DDTrace\Integrations\SandboxedIntegration;
use DDTrace\SpanData;
use DDTrace\Tag;
use DDTrace\Type;

class EloquentSandboxedIntegration extends SandboxedIntegration
labbati marked this conversation as resolved.
Show resolved Hide resolved
{
const NAME = 'eloquent';

/**
* @var string The app name. Note that this value is used as a cache, you should use method getAppName().
*/
private $appName;

/**
* {@inheritdoc}
*/
public function getName()
{
return self::NAME;
}

/**
* {@inheritDoc}
*/
public function init()
{
$integration = $this;

dd_trace_method(
'Illuminate\Database\Eloquent\Builder',
'getModels',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.get';
$sql = $this->getQuery()->toSql();
$span->resource = $sql;
$span->meta[Tag::DB_STATEMENT] = $sql;
$integration->setCommonValues($span);
}
);

dd_trace_method(
'Illuminate\Database\Eloquent\Model',
'performInsert',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.insert';
$span->resource = get_class($this);
$integration->setCommonValues($span);
}
);

dd_trace_method(
'Illuminate\Database\Eloquent\Model',
'performUpdate',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.update';
$span->resource = get_class($this);
$integration->setCommonValues($span);
}
);

dd_trace_method(
'Illuminate\Database\Eloquent\Model',
'delete',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.delete';
$span->resource = get_class($this);
$integration->setCommonValues($span);
}
);

dd_trace_method(
'Illuminate\Database\Eloquent\Model',
'destroy',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.destroy';
$span->resource = get_called_class();
$integration->setCommonValues($span);
}
);

dd_trace_method(
'Illuminate\Database\Eloquent\Model',
'refresh',
function (SpanData $span) use ($integration) {
$span->name = 'eloquent.refresh';
$span->resource = get_class($this);
$integration->setCommonValues($span);
}
);

return Integration::LOADED;
}

/**
* Set common values shared by many different spans.
*
* @param SpanData $span
*/
public function setCommonValues(SpanData $span)
{
$span->type = Type::SQL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing the service name here. We'll need to add something similar to this method as a static method and then add:

$span->service = EloquentSandboxedIntegration::getAppName();

Or grab it in the constructor to reduce the number of method calls and then we could just do something like:

$span->service = $this->serviceName;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch @SammyK

$span->service = $this->getAppName();
$span->meta[Tag::INTEGRATION_NAME] = $this->getName();
}

/**
* @return string
*/
public function getAppName()
{
if (null !== $this->appName) {
return $this->appName;
}

$name = Configuration::get()->appName();
if (empty($name) && is_callable('config')) {
$name = config('app.name');
}

$this->appName = $name ?: 'laravel';
return $this->appName;
}
}
1 change: 0 additions & 1 deletion src/DDTrace/Integrations/Integration.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ abstract class Integration
*/
abstract public function getName();


public function __construct()
{
$this->configuration = $this->buildConfiguration();
Expand Down
6 changes: 5 additions & 1 deletion src/DDTrace/Integrations/IntegrationsLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use DDTrace\Integrations\Curl\CurlIntegration;
use DDTrace\Integrations\ElasticSearch\V1\ElasticSearchIntegration;
use DDTrace\Integrations\Eloquent\EloquentIntegration;
use DDTrace\Integrations\Eloquent\EloquentSandboxedIntegration;
use DDTrace\Integrations\Guzzle\GuzzleIntegration;
use DDTrace\Integrations\Laravel\LaravelIntegration;
use DDTrace\Integrations\Lumen\LumenIntegration;
Expand Down Expand Up @@ -74,7 +75,10 @@ public function __construct(array $integrations)
$this->integrations = $integrations;
// Sandboxed integrations get loaded with a feature flag
if (Configuration::get()->isSandboxEnabled()) {
$this->integrations[PDOSandboxedIntegration::NAME] = '\DDTrace\Integrations\PDO\PDOSandboxedIntegration';
$this->integrations[EloquentSandboxedIntegration::NAME] =
'\DDTrace\Integrations\Eloquent\EloquentSandboxedIntegration';
$this->integrations[PDOSandboxedIntegration::NAME] =
'\DDTrace\Integrations\PDO\PDOSandboxedIntegration';
}
}

Expand Down
1 change: 1 addition & 0 deletions src/DDTrace/Tag.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class Tag
const ERROR_MSG = 'error.msg'; // string representing the error message
const ERROR_TYPE = 'error.type'; // string representing the type of the error
const ERROR_STACK = 'error.stack'; // human readable version of the stack
const INTEGRATION_NAME = 'integration.name';
const HTTP_METHOD = 'http.method';
const HTTP_STATUS_CODE = 'http.status_code';
const HTTP_URL = 'http.url';
Expand Down
35 changes: 34 additions & 1 deletion tests/Common/IntegrationTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace DDTrace\Tests\Common;

use DDTrace\Integrations\IntegrationsLoader;
use DDTrace\Util\Versions;
use PHPUnit\Framework\TestCase;

/**
Expand All @@ -11,15 +13,46 @@ abstract class IntegrationTestCase extends TestCase
{
use TracerTestTrait, SpanAssertionTrait;

const IS_SANDBOX = false;
labbati marked this conversation as resolved.
Show resolved Hide resolved

public static function setUpBeforeClass()
{
parent::setUpBeforeClass();
if (!static::isSandboxed()) {
putenv('DD_TRACE_SANDBOX_ENABLED=false');
}
IntegrationsLoader::reload();
}

public static function tearDownAfterClass()
{
parent::tearDownAfterClass();
putenv('DD_TRACE_SANDBOX_ENABLED');
}

protected static function isSandboxed()
{
return static::IS_SANDBOX === true;
}

protected function setUp()
{
parent::setUp();
if (Versions::phpVersionMatches('5.4') && self::isSandboxed()) {
$this->markTestSkipped('Sandboxed tests are skipped on PHP 5.4.');
}
}

/**
* Checks the exact match of a set of SpanAssertion with the provided Spans.
*
* @param array[] $traces
* @param SpanAssertion[] $expectedSpans
* @param bool $isSandbox
*/
public function assertSpans($traces, $expectedSpans, $isSandbox = false)
public function assertSpans($traces, $expectedSpans, $isSandbox = null)
{
$isSandbox = null === $isSandbox ? self::isSandboxed() : $isSandbox;
$this->assertExpectedSpans($traces, $expectedSpans, $isSandbox);
}

Expand Down
4 changes: 2 additions & 2 deletions tests/Common/SpanAssertionTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ public function assertOneExpectedSpan($traces, SpanAssertion $expectedSpan)
{
$spanChecker = new SpanChecker();

$found = array_filter($spanChecker->flattenTraces($traces), function ($span) use ($expectedSpan) {
$found = array_values(array_filter($spanChecker->flattenTraces($traces), function ($span) use ($expectedSpan) {
return $span['name'] === $expectedSpan->getOperationName();
});
}));

if (empty($found)) {
TestCase::fail('Span not found in traces: ' . $expectedSpan->getOperationName());
Expand Down
6 changes: 4 additions & 2 deletions tests/Common/SpanChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ public function assertSpan($span, SpanAssertion $exp)
{
TestCase::assertNotNull($span, 'Expected span was not \'' . $exp->getOperationName() . '\' found.');

$spanMeta = isset($span['meta']) ? $span['meta'] : [];

if ($exp->isOnlyCheckExistence()) {
return;
}
Expand All @@ -81,7 +83,7 @@ public function assertSpan($span, SpanAssertion $exp)
);
if ($exp->getExactTags() !== SpanAssertion::NOT_TESTED) {
$filtered = [];
foreach ($span['meta'] as $key => $value) {
foreach ($spanMeta as $key => $value) {
if (!in_array($key, $exp->getExistingTagNames())) {
$filtered[$key] = $value;
}
Expand Down Expand Up @@ -116,7 +118,7 @@ public function assertSpan($span, SpanAssertion $exp)
$namePrefix . "Unexpected extra values for 'tags':\n" . print_r($filtered, true)
);
foreach ($exp->getExistingTagNames(isset($span['parent_id'])) as $tagName) {
TestCase::assertArrayHasKey($tagName, $span['meta']);
TestCase::assertArrayHasKey($tagName, $spanMeta);
}
}
if ($exp->getExactMetrics() !== SpanAssertion::NOT_TESTED) {
Expand Down
2 changes: 1 addition & 1 deletion tests/Common/TracerTestTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ private function parseTracesFromDumpedData()
$span = new Span(
$rawSpan['name'],
$spanContext,
$rawSpan['service'],
isset($rawSpan['service']) ? $rawSpan['service'] : null,
$rawSpan['resource'],
$rawSpan['start']
);
Expand Down