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.

Sign up for GitHub

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

Jump to bottom

MongoDB legacy integration #166

Merged
merged 18 commits into from
Dec 5, 2018
Merged

MongoDB legacy integration #166

merged 18 commits into from
Dec 5, 2018

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Dec 4, 2018 •
edited
Loading

Description

Adds an integration for the legacy mongo extension. So far this traces the following classes:

  • MongoClient
  • MongoDB
  • MongoCollection

If you think we need to trace more than this, let me know. Also, this tags the BSON MongoID where it finds it and I'm not sure if we want to trace the ID or not. Just let me know! :)

Readiness checklist

@SammyK SammyK added the 🎉 new-integration A new integration label Dec 4, 2018
@SammyK SammyK requested review from pawelchcki and labbati December 4, 2018 01:11
@SammyK
Copy link
Contributor Author

SammyK commented Dec 4, 2018

It looks like the CI tests are passing because the mongo extension I tried to pull in for 5.6 isn't working so it's skipping them. Thoughts? :)

labbati
labbati requested changes Dec 4, 2018
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Hey @SammyK awesome job. A lot of very useful info will be added to our traces. I love it 😄 .
I added a few comments if they make sense to you.

.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoClientIntegration.php Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoClientIntegration.php Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoIntegration.php Show resolved Hide resolved
@SammyK SammyK force-pushed the sammyk/integration-mongodb-legacy branch from b9d1338 to 63d48ea Compare December 4, 2018 15:08
@SammyK
Copy link
Contributor Author

SammyK commented Dec 4, 2018 •
edited by labbati
Loading

I've updated the PR based on the feedback - thanks @labbati! :) The PHP 5.6 docker image still doesn't seem to pull in the latest version with the mongo extension installed. Any thoughts on getting that to work? :)

Yes. Dockerhub images build does not trigger automatically. Just triggered it, it may take up to 30 mins

@labbati
Copy link
Member

labbati commented Dec 4, 2018

It looks great, just one thing from the top of my head @SammyK. Did you try to send some real traces tot he UI? How many "levels" of tracing you see? Just to make sure that we are not tracing too much. E.g. in the case of the methods that we trace are mutually exclusive, so the traces only go no more than 2/3 levels. Just curious if you played with it.

@SammyK SammyK force-pushed the sammyk/integration-mongodb-legacy branch 2 times, most recently from aea1efd to b379211 Compare December 4, 2018 16:25
labbati
labbati requested changes Dec 5, 2018
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Added a few more comments to the PR, also based on the integrations enabling/disabling feature we just merged into master

src/DDTrace/Integrations/Laravel/V4/LaravelProvider.php Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoIntegration.php Show resolved Hide resolved
tests/Integration/Integrations/Mongo/MongoTest.php Outdated Show resolved Hide resolved
tests/Integration/Integrations/Mongo/MongoTest.php Outdated Show resolved Hide resolved
SammyK added 17 commits December 5, 2018 08:54
@SammyK
Add MongoDB Docker container
316b067
@SammyK
Add initial ext/mongo integration
9f4fdd9
@SammyK
Add DSN string obfuscation
2be928e
@SammyK
Extract database name from DSN
a6c08c5
@SammyK
Return null instead of the DSN
f9d5aa4
@SammyK
Add MongoDB integration
000f710
@SammyK
Add more methods to trace for MongoClient
f090b64
@SammyK
Add MongoCollection integration
a1c8a69
@SammyK
Update CHANGELOG
1a8d2c3
@SammyK
Switch MongoDB docker image to circleci/mongo
5c92196
@SammyK
Add more comments to Mongo integration
0dfe7ac
@SammyK
Update Span references to DDTrace versions
3d92416
@SammyK
Fix "Undefined offset: 1" errors
8f2e858
@SammyK
Remove deprecated methods from Mongo integration
ad89ee1
@SammyK
Add Mongo integration into IntegrationsLoader
2cda4b1
@SammyK
Move PHP-5-only tests to their own directory
00bc29c
@SammyK
Remove check for mongo extension since we want to assume it is always…
45a7c4b 
… there for CI checks
@SammyK SammyK force-pushed the sammyk/integration-mongodb-legacy branch from b379211 to 45a7c4b Compare December 5, 2018 14:34
@SammyK
Copy link
Contributor Author

SammyK commented Dec 5, 2018

I think I came up with an elegant solution to our skip-tests woes. I created a new tests/IntegrationPHP5 directory that only gets run on PHP 5 builds. Now when you run the tests on PHP 7, you don't get a huge list of skipped tests. :)

I also refactored this based on the new auto loading feature. :)

@SammyK
Copy link
Contributor Author

SammyK commented Dec 5, 2018

Thanks for all the help with this @labbati! Here's a screenshot of the trace we did together:

screen shot 2018-12-05 at 10 46 32 am

@SammyK SammyK added this to the 0.7.0 milestone Dec 5, 2018
labbati
labbati previously approved these changes Dec 5, 2018
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

LGTM :D awesome @SammyK
I only noticed a couple of missing docblock, feels free to ignore those comments

src/DDTrace/Integrations/Mongo/MongoClientIntegration.php Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoCollectionIntegration.php Show resolved Hide resolved
src/DDTrace/Integrations/Mongo/MongoDBIntegration.php Show resolved Hide resolved
labbati
labbati approved these changes Dec 5, 2018
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Looks like we have mongodb integration 😄 good job @SammyK

pawelchcki
pawelchcki approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

💥

@SammyK SammyK merged commit 329e290 into master Dec 5, 2018
@SammyK SammyK deleted the sammyk/integration-mongodb-legacy branch December 5, 2018 16:24
@SammyK
Copy link
Contributor Author

SammyK commented Dec 5, 2018

@pawelchcki
Copy link
Contributor

image

bwoebi pushed a commit that referenced this pull request Sep 15, 2023
@Anilm3
Support PHP 8.2 Release (#166)
bf69802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawelchcki pawelchcki pawelchcki approved these changes

@labbati labbati labbati approved these changes

Assignees
No one assigned
Labels
🎉 new-integration A new integration
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@SammyK @labbati @pawelchcki