-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add SQLSRV Integration #2031
Add SQLSRV Integration #2031
Conversation
Note: The sha256 for the buster images weren't the one I'd expect (not the same as the latest versions)
This reverts commit 52d44ee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
|
||
$integration->detectError($hook->returned, $span); | ||
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, as we're only in maintenance mode for PHP5, you didn't have to do this. But no worries if it is done now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It just has no effect on PHP 5 as it's not declared in the IntegrationsLoader for PHP 5)
|
||
protected function setMetricNumRows(SpanData $span, $stmt) | ||
{ | ||
if ($stmt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no num_rows, can you please assign sqlsrv_rows_affected() to db.row_count? (like in the PDO integration, where both are merged into the same function incidentally; the mysqlintegration is a bit wrong on that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me.
I'm just a bit confused where the logic is to not build sqlsrv on arm64 for older versions? I thought we found that the sqlsrv extension does not work on older PHP versions with arm64, or do I misremember?
Description
This PR adds a SQLSRV Integration. Moreover, it will change the buster dockerfiles to include the required drivers (these images have already been pushed to datadog/dd-trace-ci)
Readiness checklist
Reviewer checklist