Migrate eloquent integration to sandboxed API #559
Conversation
| $this->connection()->exec("insert into users (email) VALUES ('test-user-updated@email.com')"); | ||
| $traces = $this->tracesFromWebRequest(function () { | ||
| $spec = GetSpec::create('Eloquent update', '/eloquent/update'); | ||
| $response = $this->call($spec); |
There was a problem hiding this comment.
Does $response need to get used somehow?
There was a problem hiding this comment.
It was just useful to print it out, let me remove from everywhere though, good idea
SammyK
force-pushed
the
sammyk/sandboxed-pdo
branch
3 times, most recently
from
c1ccc02
to
b6d378a
Compare
labbati
force-pushed
the
labbati/sandboxing-eloquent
branch
from
caf9d48
to
cae7075
Compare
SammyK
force-pushed
the
sammyk/sandboxed-pdo
branch
4 times, most recently
from
24922f1
to
79ae3fd
Compare
labbati
force-pushed
the
labbati/sandboxing-eloquent
branch
2 times, most recently
from
dc9e446
to
c6c3b53
Compare
labbati
changed the base branch from
sammyk/sandboxed-pdo
to
master_disabled_pdo
labbati
force-pushed
the
labbati/sandboxing-eloquent
branch
from
5fdd340
to
a6c318d
Compare
…andboxed eloquent tests
…fining IS_SANDEBOXED
labbati
changed the title
labbati
added
🎉 new-integration
There was a problem hiding this comment.
Fantastic sandboxed integration, @labbati! I really love how much cleaner you made the tests overall.
I have a few small tweaks but the main thing was making sure the service name gets set in the sandboxed integration. The Agent drops any spans that are missing the service name. But other than that, this PR is totally on point! ❤️
src/DDTrace/Integrations/Eloquent/EloquentSandboxedIntegration.php
Outdated
Show resolved
Hide resolved
src/DDTrace/Integrations/Eloquent/EloquentSandboxedIntegration.php
Outdated
Show resolved
Hide resolved
| */ | ||
| public function setCommonValues(SpanData $span) | ||
| { | ||
| $span->type = Type::SQL; |
There was a problem hiding this comment.
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;|
|
||
| protected function expectedServiceName() | ||
| { | ||
| // Shouldn't this not be set by teh span encoder? |
There was a problem hiding this comment.
The SpanEncoder is only used for userland spans. C-level spans run through the serializer. Once we get the service name worked out (from the earlier comment) the service names should be the same for sandboxed and non-sandboxed integrations. :)
Description
Readiness checklist
Reviewer checklist