Fixes and Improvements to botocore instrumentation by NathanielRN · Pull Request #150 · open-telemetry/opentelemetry-python-contrib

NathanielRN · 2020-11-05T21:19:55Z

Description

This PR aims to update the botocore instrumentation by doing the following:

Fixes:

Removed code that modified span.resource since resource should be immutable
Change SpanKind from CONSUMER to CLIENT because botocore api calls can be considered remote outgoing and synchronous requests requests as laid out in the SpanKind specifications - Important so that AWS services are treated as separate services in Tracing Backends. (See picture below!)
Use instrumentation.utils.unwrap instead of creating the function again

Simplifications:

Remove code that searches for nested attributes using deep_getattr and instead pulls values from properties of the botocore.BaseClient class since they should always be there
Removed params from attributes. There is a PR to add more specific semantic conventions for which params should be trace in AWS SDKs in the specifications so the plan was to revisit when it gets merged

Improvements:

More comprehensive logic to parse out request_id if it can be found in the headers instead of the usual location. Just like .NET botocore instrumentation
Add aws.table_name and aws.queue_url for DynamoDB and SQS specifically as traced properties (this will be added in the specifications)
Format tests checking of attributes to look nice!

Type of change

Please delete options that are not relevant.

Breaking change (fix or feature that would cause existing functionality to not work as expected) - Not all params are recorded now
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

With the change to SpanKind.CLIENT, S3 (and other services) are considered their own services

Before:

After:

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

NathanielRN · 2020-11-07T02:45:16Z

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py

-        self.assertEqual(spans[1].attributes["params.Key"], str(params["Key"]))
+        self.assertTrue("params.Body" not in spans[1].attributes.keys())
+        get_object_attributes = dict(spans[2].attributes)
        self.assertEqual(


I'm not sure why RequestId doesn't return here, but I raised an issue to ask if my setup is wrong.

getmoto/moto#3442

Got a reply confirming that the RequestId is missing because its simply not added to the S3 "mock" of the service in moto, so this is the best we can do right now :)

NathanielRN · 2020-11-13T19:10:41Z

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py

+            {
+                "aws.operation": "DescribeInstances",
+                "aws.region": "us-west-2",
+                "aws.request_id": "fdcdcab1-ae5c-489e-9c33-4637c5dda355",


Not sure why the IDs are sometimes consistent and sometimes not with moto.

Sometimes the ID is consistent like this EC2 call:

{ 'Reservations': [], 'ResponseMetadata': {'RequestId': 'fdcdcab1-ae5c-489e-9c33-4637c5dda355', 'HTTPStatusCode': 200, 'HTTPHeaders': {'server': 'amazon.com'}, 'RetryAttempts': 0} }

and other times it's randomly generated like in this SQS call:

{ 'ResponseMetadata': { 'RequestId': 'JWH6CYKFXYT2KXQSAYY50T6FK558XMQ7U9SOWLJ5YEQZOKULQ012', 'HTTPStatusCode': 200, 'HTTPHeaders': {'server': 'amazon.com', 'x-amzn-requestid': 'JWH6CYKFXYT2KXQSAYY50T6FK558XMQ7U9SOWLJ5YEQZOKULQ012', 'x-amz-crc32': '1416772655'}, 'RetryAttempts': 0 } }

Any insights on why RequestId looks different on SQS ? my guess would be they are not generated using same lib or similar standards.

Yes! I looked into it, and found this code used to generate the request ID in the moto repository:

https://github.com/spulec/moto/blob/9db62d32bf70e18f315305b7915d199e3ba1210a/moto/core/utils.py#L238-L244

It was explained to me that this function is used by some of the resources to attach a request ID.

Notably the amzn_request_id() decorator is used on SQS and DynamoDB and that's why their request IDs look like this 🙂

And some services have hardcoded requests like EC2 DescribeInstances (https://github.com/spulec/moto/blob/50d71eccbee12ce5cf2c1ecf61bfcbd60317976d/moto/ec2/responses/instances.py#L479)

I'm not sure why it doesn't format the request IDs in the expected way (lower case joined with hyphens) because it used to be correct.

In that case, should I remove the regex check and just make it a check for the key? Let me know what you think!

bhautikpip · 2020-11-13T20:54:58Z

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py

+            {
+                "aws.operation": "DescribeInstances",
+                "aws.region": "us-west-2",
+                "aws.request_id": "fdcdcab1-ae5c-489e-9c33-4637c5dda355",


Any insights on why RequestId looks different on SQS ? my guess would be they are not generated using same lib or similar standards.

bhautikpip · 2020-11-13T20:59:04Z

...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py

-                ("action", "params", "path", "verb"),
-                {"params", "path", "verb"},
-            )
+            error = None


declare these values at the top ?

Sure can do. I guess I thought it would be better to leave them down here though since they don't need to be in scope at the top?

bhautikpip · 2020-11-13T21:01:10Z

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py

+            span.attributes,
+            {
+                "aws.operation": "ListObjects",
+                "aws.region": "us-west-2",


Any reason on why don't we validate status code?

Because this is a ParamValidationError the botocore library will never send the HTTP request and so there's no http.status_code to check!

But that's a good point! I can add a check to make sure this span was recorded as an error.

self.assertIs( span.status.status_code, trace_api.status.StatusCode.ERROR, )

instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py

...tation/opentelemetry-instrumentation-boto/src/opentelemetry/instrumentation/boto/__init__.py

owais · 2020-11-13T19:23:39Z

...tation/opentelemetry-instrumentation-boto/src/opentelemetry/instrumentation/boto/__init__.py

+            }
+            if isinstance(dict_, dict)
+            else {prefix: dict_}
+        )


nit: looks like these functions don't depend on anything from the outer context and take everything they need as arguments so do we really need them to be defined in line?

This should probably be moved into opentelemetry.instrumentation.utils anyways. I'll keep the Stack Overflow comment and add a comment about it being better suited as a util function for now.

owais · 2020-11-13T19:24:55Z

...tation/opentelemetry-instrumentation-boto/src/opentelemetry/instrumentation/boto/__init__.py

+    if not span.is_recording():
+        return
+
+    if endpoint_name not in {"kms", "sts"}:


I think a comment here explaining why this check exists would be nice.

Sorry I just copied this code because I didn't want to change the boto functionality while updating botocore. I can only guess that it's because these are the Key Management Service and Secure Token Service services so maybe their API trace calls should not be traced?

I added a comment mentioning that but I can't guess pass that. It doesn't look like I the commit history says who added these in the first place either.

owais · 2020-11-16T18:02:41Z

...tation/opentelemetry-instrumentation-boto/src/opentelemetry/instrumentation/boto/__init__.py

+        for key, value in {
+            k: truncate_arg_value(v)
+            for k, v in tags.items()
+            if k not in {"s3": ["params.Body"]}.get(endpoint_name, [])


This is a bit cryptic and intention might not be very clear. Mind breaking it down or simplifying? Looks we really want to check something like if endpoint_name == 's3' and k == 'params.Body'? Personally, I'd write it out as a regular loop clearly self-documenting the intent and separating logic to multiple lines.

I really agree with you, thanks for the tip! I've updated it to look cleaner, please let me know what you think!

...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py

bhautikpip

LGTM :)

NathanielRN requested review from a team, codeboten and owais and removed request for a team November 5, 2020 21:19

NathanielRN force-pushed the update-botocore-instrumentation branch from e566384 to f098c5c Compare November 5, 2020 21:22

NathanielRN marked this pull request as draft November 5, 2020 23:42

NathanielRN force-pushed the update-botocore-instrumentation branch 7 times, most recently from 27fbdaa to 2473eff Compare November 7, 2020 02:34

NathanielRN commented Nov 7, 2020

View reviewed changes

NathanielRN marked this pull request as ready for review November 7, 2020 02:46

NathanielRN force-pushed the update-botocore-instrumentation branch 3 times, most recently from cb661e3 to b1ac40f Compare November 12, 2020 23:13

NathanielRN changed the title ~~Simplify attributes for botocore instrumentation~~ Fixes and Improvements to botocore instrumentation Nov 13, 2020

NathanielRN force-pushed the update-botocore-instrumentation branch from b1ac40f to b423b0c Compare November 13, 2020 18:45

NathanielRN commented Nov 13, 2020

View reviewed changes

bhautikpip reviewed Nov 13, 2020

View reviewed changes

NathanielRN force-pushed the update-botocore-instrumentation branch from b423b0c to 9b90b01 Compare November 14, 2020 01:22

owais reviewed Nov 16, 2020

View reviewed changes

NathanielRN requested a review from owais November 16, 2020 19:23

NathanielRN force-pushed the update-botocore-instrumentation branch 2 times, most recently from 0144715 to 1acf758 Compare November 17, 2020 20:50

bhautikpip approved these changes Nov 17, 2020

View reviewed changes

NathanielRN force-pushed the update-botocore-instrumentation branch 2 times, most recently from c9251c3 to 9fe220e Compare November 17, 2020 22:21

NathanielRN added 3 commits November 18, 2020 09:15

Fixes and improvements to botocore instrumentation

d353486

Simplify filtering of boto span attributes

457634f

Add comment about security guards

467e513

NathanielRN force-pushed the update-botocore-instrumentation branch from 9fe220e to 467e513 Compare November 18, 2020 17:15

codeboten approved these changes Nov 18, 2020

View reviewed changes

codeboten merged commit fd493f4 into open-telemetry:master Nov 18, 2020

NathanielRN deleted the update-botocore-instrumentation branch November 25, 2020 18:13

thomasdesr mentioned this pull request Dec 24, 2020

Adds back retry_attempts to botocore #275

Merged

7 tasks

NathanielRN mentioned this pull request Jul 8, 2021

REQUEST: New membership for NathanielRN open-telemetry/community#777

Closed

6 tasks

Conversation

NathanielRN commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

NathanielRN Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanielRN Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhautikpip left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NathanielRN commented Nov 5, 2020 •

edited

Loading

NathanielRN Nov 7, 2020 •

edited

Loading

NathanielRN Nov 13, 2020 •

edited

Loading