Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Add details about aggregation pipelines & avoid get(default) parameter#644

Merged
reyang merged 2 commits intocensus-instrumentation:masterfrom
angelini:master
May 8, 2019
Merged

Add details about aggregation pipelines & avoid get(default) parameter#644
reyang merged 2 commits intocensus-instrumentation:masterfrom
angelini:master

Conversation

@angelini
Copy link
Copy Markdown
Contributor

@angelini angelini commented May 6, 2019

When issuing aggregations via Pymongo, they store the pipeline as a command attribute.

This PR adds support for storing the pipeline as a trace attribute.

We've also seen the following errors in production with a Mongo 3.6.12 instance an Pymongo 3.6.1.

Traceback (most recent call last): 
  File "<REDACTED>/.virtualenv/lib/python3.7/site-packages/pymongo-3.6.1-py3.7-linux-x86_64.egg/pymongo/monitoring.py", line 739, in publish_command_start 
    subscriber.started(event) 
  File "<REDACTED>/.virtualenv/lib/python3.7/site-packages/opencensus_ext_pymongo-0.1.2-py3.7.egg/opencensus/ext/pymongo/trace.py", line 54, in started 
    _attr = event.command.get(attr, default=None) 
TypeError: get() takes no keyword arguments 

It seems like not all event.command objects are SON instances (https://github.com/mongodb/mongo-python-driver/blob/master/bson/son.py#L32).

Since the default keyword argument isn't being used here, the default value is None, I opted to remove the use of it.

@angelini angelini requested review from a team, c24t, reyang and songy23 as code owners May 6, 2019 09:12
@reyang
Copy link
Copy Markdown
Contributor

reyang commented May 6, 2019

@tjzo please help to review.

Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement @angelini!
LGTM, let's wait for one day to see if any feedbacks from the original author @tjzo.

Copy link
Copy Markdown
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.

It seems like not all event.command objects are SON instances

It looks like this PR doesn't fix this? @angelini if you've got a sample command to reproduce the error we can open another ticket to track this.

def get(self, key):
return self.command_attrs[key] \
if key in self.command_attrs else default
if key in self.command_attrs else None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: might as well use self.command_attrs.get(key).

@angelini
Copy link
Copy Markdown
Contributor Author

angelini commented May 7, 2019

It seems like not all event.command objects are SON instances

It looks like this PR doesn't fix this?

As best I can tell the commands aren't SON objects, but they have a get method. This change has fixed it for our production use.

if you've got a sample command to reproduce the error we can open another ticket to track this.

Yeah that would be great, working on a getting one but so far we've only seen this in production and haven't been able to reproduce it locally.

@angelini
Copy link
Copy Markdown
Contributor Author

angelini commented May 7, 2019

@c24t I was just able to confirm from production that the command causing the error is an empty dictionary.

class DetectIncorrectCommand(pymongo.monitoring.CommandListener):
    def started(self, event):
        if not isinstance(event.command, bson.SON):
            print('command for (%s) %s' % (type(event.command), event.command))

Resulted in:

> command for (<class 'dict'>) {}

And this is the same error we were seeing.

In [1]: {}.get('a', 1)
Out[1]: 1

In [2]: {}.get('a', default=1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-9332c12c9211> in <module>
----> 1 {}.get('a', default=1)

TypeError: get() takes no keyword arguments

Removing the use of the keyword arg (default=None) https://github.com/census-instrumentation/opencensus-python/pull/644/files#diff-bb0ccb334b607c4945ac2a1ed4ac9776R54 fixes it.

@reyang
Copy link
Copy Markdown
Contributor

reyang commented May 8, 2019

@angelini would you update the CHANGELOG.md and rebase to the latest master branch? Once done, we're ready to merge.

@angelini
Copy link
Copy Markdown
Contributor Author

angelini commented May 8, 2019

@reyang it's been rebased and the changelog has been updated

@reyang reyang merged commit b28a83f into census-instrumentation:master May 8, 2019
@reyang
Copy link
Copy Markdown
Contributor

reyang commented May 8, 2019

Thanks for your contribution! @angelini

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants