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

Adding pymongo integration #232

Merged
merged 27 commits into from
Nov 7, 2019
Merged

Adding pymongo integration #232

merged 27 commits into from
Nov 7, 2019

Conversation

@Oberon00
Copy link
Member

Is this based on OpenCensus, or OpenTracing, or is it a completely new implementation?

@hectorhdzg
Copy link
Member Author

hectorhdzg commented Oct 22, 2019

@Oberon00 I took OpenCensus as starting point, I could not find any pymongo integration for OpenTracing, some properties are different and we are getting more information in the span attributes now.
https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-pymongo

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@hectorhdzg
Copy link
Member Author

@Oberon00 let me know if you have more feedback

@hectorhdzg
Copy link
Member Author

@lzchen , @Oberon00 I updated the code to have an internal span dictionary like Christian suggested, keeping track of current span and avoid conflicts with other spans created in different MongoDB clients, thanks for reviewing and let me know if you have any feedback

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Thank you @hectorhdzg, I think the code looks quite solid now.

Unfortunately, I have to request changes yet again because I'm pretty sure the unit tests aren't being run in the CI (you can verify locally by running just tox without the -e option and checking the output at the end for a line like py37-test-ext-pymongo: commands succeeded).

for attr in COMMAND_ATTRIBUTES:
_attr = event.command.get(attr)
if _attr is not None:
span.set_attribute("db.mongo." + attr, str(_attr))
Copy link
Member

Choose a reason for hiding this comment

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

On using str: Shouldn't e.g. limit be an integer?

Copy link
Member Author

Choose a reason for hiding this comment

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

string casting is more important for other fields, dictionaries and lists are also part of COMMAND_ATTRIBUTES, I can add specific checks for limit and skip(both int) if the type is important here

Copy link
Member

Choose a reason for hiding this comment

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

I think at this point, it would be simpler to take "limit" out of COMMAND_ATTRIBUTES and query it separately.

tox.ini Show resolved Hide resolved
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thanks for going through all these rounds of comments!

@Oberon00
Copy link
Member

Oberon00 commented Nov 4, 2019

@open-telemetry/python-approvers: If someone has knowledge of MongoDB (I don't), I'd love another review on this before merging.

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #232 into master will decrease coverage by 0.73%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
- Coverage    85.5%   84.77%   -0.74%     
==========================================
  Files          33       33              
  Lines        1608     1589      -19     
  Branches      181      180       -1     
==========================================
- Hits         1375     1347      -28     
- Misses        186      195       +9     
  Partials       47       47
Impacted Files Coverage Δ
...try-api/src/opentelemetry/context/async_context.py 17.39% <0%> (-34.79%) ⬇️
...elemetry-api/src/opentelemetry/context/__init__.py 88.88% <0%> (-11.12%) ⬇️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py 68.18% <0%> (-2.66%) ⬇️
...opentelemetry/sdk/context/propagation/b3_format.py 84% <0%> (-0.62%) ⬇️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 90.14% <0%> (-0.53%) ⬇️
opentelemetry-sdk/src/opentelemetry/sdk/util.py 85.54% <0%> (-0.35%) ⬇️
...src/opentelemetry/ext/opentracing_shim/__init__.py 93.33% <0%> (-0.31%) ⬇️
...ts/src/opentelemetry/ext/http_requests/__init__.py 88.88% <0%> (-0.31%) ⬇️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py 85% <0%> (-0.19%) ⬇️
...app/src/opentelemetry_example_app/flask_example.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e95a115...f2c344e. Read the comment docs.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM after fixing version numbers, but my review is taking a lot of the expected behavior on faith. It would be helpful to have an example or integration test here, or a better description in the package readme.

I agree with @Oberon00 that we'd benefit from a reviewer familiar with mongo, but don't want to hold this PR up.

ext/opentelemetry-ext-pymongo/setup.cfg Outdated Show resolved Hide resolved
@c24t c24t merged commit 9fd4ccd into open-telemetry:master Nov 7, 2019
@c24t c24t mentioned this pull request Dec 16, 2019
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
… (open-telemetry#232)

* fix: enable async hooks manager in node tracer

* fix(node-tracer): set scope manager as optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants