Boto / Botocore integration #209

raphaelgavache · 2017-03-02T16:04:34Z

# Botocore and Boto integrations

Summary

Adds botocore and boto contribs with testing units:
boto : s3/ec2/sqs/lambda
botocore : s3/ec2/sqs/lambda/kinesis
Boto3 relies on botocore so it will also be traced

Compatibility

boto >=2.29.0
botocore >= 1.4.51

TODOS

Miscellanous notes

botocore is really clean, boto is a mess:
- the first has one big class that customises itself in function of what service is called. The methods are created from JSON and the result has a standard format shared by all services.
- boto has one specific class for each AWS and which can inherit from 2 different classes AWSQueryConnection (ec2 connector for exemple) and AWSAuthConnection (s3 connector ...). Both classes make_request() methods are patched but they do not contain the same metadata. For exemple AWSAuthConnection instance does not contain the operation name called on the s3 AWS.
boto both class AWSQueryConnection and AWSAuthConnection have a region attribute. For some it's a string, others have different classes (sqs has his own class...) but there's also a standard Region class -> this is why I added a function get_region_name

inspired from https://github.com/DataDog/botocore/pull/4/files, which has been tested on staging (see https://dd.datad0g.com/trace/view/177373499236743437)

…e-py into raphael/contrib-boto

… into raphael/contrib-boto

talwai · 2017-03-06T16:52:02Z

ddtrace/contrib/boto/patch.py

+
+        # Original func returns a boto.connection.HPPResponse object
+        result = original_func(*args, **kwargs)
+        span.set_tag(http.STATUS_CODE, getattr(result, "status", 500))


let's not have it default to 500. This is a meaningful error code that influences downstream stats collection and error reporting/alerting so we do not want to set it just because there's no better candidate. Instead it's fine to set the STATUS_CODE meta only when result.status is available

talwai · 2017-03-06T16:53:33Z

ddtrace/contrib/boto/patch.py

+        return result
+
+def get_region_name(region):
+    if type(region) == str:


use isinstance

talwai · 2017-03-06T17:09:13Z

ddtrace/contrib/botocore/patch.py

+
+def patch():
+    # Checking for the version compatibility before patching
+    if LooseVersion(pkg_resources.get_distribution("botocore").version) >= LooseVersion("1.4.51"):


i don't think we want version matching code at this layer. instead let the patching function fail if a patchable isn't available - and patch_all will respond to this gracefully

talwai · 2017-03-06T17:17:06Z

ddtrace/contrib/boto/patch.py

+
+# s3, lambda
+def patched_auth_request(original_func, instance, args, kwargs):
+    tracer = getattr(instance, 'datadog_tracer', ddtrace.tracer)


instead use a Pin

e.g. https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/redis/patch.py#L51-L53

talwai · 2017-03-08T19:23:52Z

ddtrace/contrib/boto/patch.py

+
+    with pin.tracer.trace('boto.command', service=DEFAULT_SERVICE, span_type=SPAN_TYPE) as span:
+
+        # Original func returns a boto.connection.HPPResponse object


talwai · 2017-03-08T19:24:36Z

ddtrace/contrib/boto/patch.py

+        # Original func returns a boto.connection.HPPResponse object
+        result = original_func(*args, **kwargs)
+        span.set_tag(http.STATUS_CODE, getattr(result, "status"))
+        span.set_tag(http.METHOD, getattr(result, "_method", "unknown"))


rather than set "unknown", it's ok to leave metadata unset in general when the needed attribute is not available

talwai · 2017-03-08T19:42:51Z

tests/contrib/boto/test.py

+
+        # Testing resource and service
+        eq_(span.service, "aws")
+        eq_(span.resource, "boto.command")


this is not the correct resource. update this to do something similar to what botocore does.
and also make sure the check span.name in this test

talwai · 2017-03-08T19:43:28Z

tests/contrib/boto/test.py

+        eq_(span.get_tag(http.STATUS_CODE), "200")
+        eq_(span.get_tag(http.METHOD), "GET")
+        eq_(span.get_tag('aws.endpoint'), "lambda")
+        eq_(span.get_tag('aws.region'), "us-east-2")


need to check span.service , span.resource and span.name in all these tests

b1-88er · 2017-03-13T12:32:24Z

tests/contrib/boto/test.py

+        s3.get_all_buckets()
+        spans = writer.pop()
+        assert spans
+        span = spans[0]


does length of spans matter here?

one span is generated each time the function patched is called. For each request of the S3 client https://github.com/boto/boto/blob/develop/boto/s3/connection.py#L440, patched function gets called only once. On tests on our staging env there's a unique trace per s3 call. Yet in the mocking env I get 3 spans, I'll investigate on that

talwai · 2017-03-16T15:14:53Z

ddtrace/contrib/boto/patch.py

+    operation_name = None
+    for trace in reversed(traceback.extract_stack()):
+        # Going backwards in the traceback till first call outside off ddtrace before make_request
+        if "ddtrace" not in trace[0].split('/') and trace[2] != 'make_request':


this code worries me. if there's no more reliable way to pull an operation_name, we should at least have a length check on the stacktrace before we try to index into it arbitrarily.

talwai · 2017-03-16T15:22:32Z

ddtrace/ext/aws.py

+    if endpoint_name in BLACKLIST_ENDPOINT:
+        return True
+    else:
+        return False


just return endpoint_name in BLACKLIST_ENDPOINT

talwai · 2017-03-16T15:23:42Z

ddtrace/ext/aws.py

+    else:
+        return False
+
+def unpacking_args(args, args_name, traced_args_list):


this needs a docstring

tmichelet and others added 7 commits October 4, 2016 16:26

[Contrib] Botocore - first pass of a botocore contrib

55c8635

inspired from https://github.com/DataDog/botocore/pull/4/files, which has been tested on staging (see https://dd.datad0g.com/trace/view/177373499236743437)

Merge branch 'tristan/contrib-botocore' of github.com:DataDog/dd-trac…

3cb4df8

…e-py into raphael/contrib-boto

First commit, botocore contrib (tests working on local machine)

3e5fa76

Contrib test taken into account

3749696

fixing contrib patching issue

f243c68

should pass contrib test

b91d6da

Botocore with tests runing on circleCi

14fee39

clutchski mentioned this pull request Mar 2, 2017

[Contrib] Botocore - first pass of a botocore contrib #72

Closed

3 tasks

Raphael Gavache and others added 7 commits March 2, 2017 12:43

adding http status code and number of retry attempts

ed7ccd6

Adding boto integration

56eb079

Merge branch 'master' into raphael/contrib-boto

60b1bfc

added http.STATUS_CODE and http.METHOD

3ffaf72

Merge branch 'raphael/contrib-boto' of github.com:DataDog/dd-trace-py…

330e91f

… into raphael/contrib-boto

sqs/lambda tests on boto (tests on Circleci)`

270712b

adding mocked AWS calls to botocore tests

19ccd45

raphaelgavache requested a review from talwai March 3, 2017 21:57

talwai reviewed Mar 6, 2017

View reviewed changes

ddtrace/contrib/boto/patch.py Outdated

return result

def get_region_name(region):

if type(region) == str:

Copy link

Contributor

talwai Mar 6, 2017

Choose a reason for hiding this comment

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

use isinstance

talwai reviewed Mar 6, 2017

View reviewed changes

talwai changed the title ~~Raphael/contrib boto~~ Boto / Botocore integration Mar 6, 2017

talwai reviewed Mar 6, 2017

View reviewed changes

Raphael Gavache added 7 commits March 6, 2017 16:48

Pin ddagent, adding tests and nps

3e63ace

small fix for tests

6f28c94

adding boto/botocore in monkey default

e3b4894

fixing json.dumps errors

8860926

no more json encoding

32a29a6

corrections on names

ed79d36

Changing test for py34 success

fc5bc14

talwai reviewed Mar 8, 2017

View reviewed changes

Raphael Gavache added 3 commits March 8, 2017 17:05

Corrections after review, added aws.operation to boto s3

6a57eaa

botocore test fix

e338c5f

adding sts, kms args/kwargs protection

d975019

palazzem mentioned this pull request Mar 9, 2017

boto tracing #205

Closed

palazzem modified the milestone: 0.7.0 Mar 9, 2017

adding boto sts/kms protection

48f1b9f

palazzem added the integrations label Mar 9, 2017

Adding operation names for the rest of boto integrations

8ec2ec2

b1-88er reviewed Mar 13, 2017

View reviewed changes

Raphael Gavache and others added 8 commits March 13, 2017 17:32

changing boto args, protection, way to get the endpoint

cf3d611

protection against py34/py35 tests with diff messages

3fec0f2

upgrading boto operation name research (scalable)

3d22f92

correction on boto getting operation name

4222296

code cleaning and upgrading tests

ecc48ab

ready for test, setting autopatch to false

916aa31

flake8 correction

10769d5

ready for test, unpatched

fab3448

talwai reviewed Mar 16, 2017

View reviewed changes

ddtrace/ext/aws.py Outdated

else:

return False

def unpacking_args(args, args_name, traced_args_list):

Copy link

Contributor

talwai Mar 16, 2017

Choose a reason for hiding this comment

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

this needs a docstring

Raphael Gavache added 4 commits March 16, 2017 11:43

Aaditya's review

55e57fa

removing len(trace_frame), not working py3

5c6e262

changing traceback to inspect

af2c47e

np: readability

f394197

talwai approved these changes Mar 16, 2017

View reviewed changes

raphaelgavache merged commit 38dc43d into master Mar 16, 2017

palazzem deleted the raphael/contrib-boto branch March 16, 2017 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Boto / Botocore integration #209

Boto / Botocore integration #209

raphaelgavache commented Mar 2, 2017 •

edited

talwai Mar 6, 2017

talwai Mar 6, 2017

talwai Mar 6, 2017

talwai Mar 6, 2017

talwai Mar 8, 2017

talwai Mar 8, 2017

talwai Mar 8, 2017

talwai Mar 8, 2017

b1-88er Mar 13, 2017 •

edited

raphaelgavache Mar 15, 2017

talwai Mar 16, 2017

talwai Mar 16, 2017

talwai Mar 16, 2017


		with pin.tracer.trace('boto.command', service=DEFAULT_SERVICE, span_type=SPAN_TYPE) as span:

		# Original func returns a boto.connection.HPPResponse object

Boto / Botocore integration #209

Boto / Botocore integration #209

Conversation

raphaelgavache commented Mar 2, 2017 • edited

Summary

Compatibility

TODOS

Miscellanous notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1-88er Mar 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelgavache commented Mar 2, 2017 •

edited

b1-88er Mar 13, 2017 •

edited