Infer API Gateway spans #172

agocs · 2021-09-07T17:27:14Z

What does this PR do?

Gives dd-lambda-py the ability to infer spans for API Gateway, HTTPAPI, and Websocket invocations.

Known issue

The inferred span's service is still being overwritten with the value of the lambda function's service tag by logs backend. I think I've exhausted my options for fixing that on the lambda library side. I'll start working on a fix for the backend.

Motivation

Testing Guidelines

I've added JSON events and unit tests for

non-proxy API Gateway requests
API Gateway websocket events
and HTTP API requests

I've also deployed a lambda function in the Sandbox account, tied to the Datadog Serverless org, instrumented with this version of the code, and have been hitting it with CURL requests and via websocket in POSTMAN.

Additional Notes

Types of Changes

Bug fix
New feature
Breaking change
Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

This PR's description is comprehensive
This PR contains breaking changes that are documented in the description
This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
This PR impacts documentation, and it has been updated (or a ticket has been logged)
This PR's changes are covered by the automated tests
This PR collects user input/sensitive content into Datadog
This PR passes the integration tests (ask a Datadog member to run the tests)

agocs · 2021-09-10T20:08:39Z

n.b. @Hesperide the inferred spans are still showing up as the same color as the Lambda spans, and they're the same node on the trace map. I've tried setting the service name to the API Gateway URL (or equivalent), and it doesn't seem to want to take.

Hesperide · 2021-09-11T23:38:55Z

n.b. @Hesperide the inferred spans are still showing up as the same color as the Lambda spans, and they're the same node on the trace map. I've tried setting the service name to the API Gateway URL (or equivalent), and it doesn't seem to want to take.

How can I help unblock this? Do AJ/Darcy have any ideas?

Hesperide · 2021-09-12T01:55:15Z

README.md

@@ -87,6 +87,16 @@ Initialize the Datadog tracer when set to `true`. Defaults to `false`.

 Set to `true` to merge the X-Ray trace and the Datadog trace, when using both the X-Ray and Datadog tracing. Defaults to `false`.

+### DD_INFERRED_SPANS (beta)


I don't know if you were planning to update this later, but when we're GA this should default to true.

We should also flag as "experimental" instead of "beta".

tianchu · 2021-09-13T18:15:11Z

n.b. @Hesperide the inferred spans are still showing up as the same color as the Lambda spans, and they're the same node on the trace map. I've tried setting the service name to the API Gateway URL (or equivalent), and it doesn't seem to want to take.

How can I help unblock this? Do AJ/Darcy have any ideas?

This is probably due to the backend tag resolution overriding? I believe the backend applies the service tag value to every span in the same trace payload, because it assumes that spans in the same trace payload come from the same service. One potential solution is to put the inferred trigger span in its own trace payload, perhaps by calling tracer.write()?

agocs · 2021-09-14T17:49:25Z

This is probably due to the backend tag resolution overriding? I believe the backend applies the service tag value to every span in the same trace payload, because it assumes that spans in the same trace payload come from the same service. One potential solution is to put the inferred trigger span in its own trace payload, perhaps by calling tracer.write()?

Hey Tian, I tried explicitly calling tracer.write() and it seems to do what we think it does, but the tracer still flushes the whole trace (with both spans) at the end of the execution, to the same effect.

Update: Darcy points out that I'm sending via the forwarder, and the forwarder is probably rewriting my service names. I'll try to make this work with the extension.

…s running and the tag is not set on the function

tianchu · 2021-09-23T17:31:22Z

datadog_lambda/wrapper.py

+            self.inferred_span = None
+            self.make_inferred_span = (
+                os.environ.get("DD_INFERRED_SPANS", "false").lower() == "true"
+                and should_use_extension


Any reason to not support inferred spans when using the forwarder?

The forwarder adds tags from the tag cache to the trace payload(s) being sent back to the trace intake endpoint. This is an additional variable and an additional thing we would need to fix before the inferred spans show up as their own services. We can always remove this check and add forwarder support later.

tianchu · 2021-09-23T18:37:10Z

datadog_lambda/wrapper.py

+            if self.inferred_span:
+                if status_code:
+                    self.inferred_span.set_tag("http.status_code", status_code)
+                self.inferred_span.finish()
            logger.debug("datadog_lambda_wrapper _after() done")


This log line is no longer at the right place.

tianchu · 2021-09-23T18:38:34Z

datadog_lambda/tracing.py

 def create_function_execution_span(
    context,
    function_name,
    is_cold_start,
    trace_context_source,
    merge_xray_traces,
    trigger_tags,
+    upstream=None,


Is "parent" a better name?

tianchu · 2021-09-23T18:40:13Z

datadog_lambda/tracing.py

@@ -402,6 +546,7 @@ def create_function_execution_span(
            else None,
            "datadog_lambda": datadog_lambda_version,
            "dd_trace": ddtrace_version,
+            "span.name": "aws.lambda",


Why do you need this? Doesn't span = tracer.trace("aws.lambda", **args) (later in this function) set the span name to aws.lambda?

tianchu · 2021-09-23T18:45:58Z

datadog_lambda/tracing.py

+SPAN_TYPE_INFERRED = "inferred"
+
+
+class InferredSpanFilter(TraceFilter):


I think this class definitely deserve a comprehensive docstring to explain why it's needed and the background.

tianchu · 2021-09-23T18:49:51Z

datadog_lambda/tracing.py

+    return None
+
+
+def detect_inferrable_span_type(event):


Can you reuse (maybe some refactoring required) the same logic from https://github.com/DataDog/datadog-lambda-python/blob/main/datadog_lambda/trigger.py? Want to avoid inconsistency between what's shown by the trace and the invocation.

tianchu · 2021-09-23T18:51:32Z

datadog_lambda/tracing.py

+    tags = {
+        "operation_name": "aws.apigateway.websocket",
+        "service.name": domain,
+        "url": domain + endpoint,


datadog-lambda-python/datadog_lambda/trigger.py

Line 154 in c86be18

http_tags["http.url"] = request_context["domainName"]

sounds like the standard field is http.url?

DarcyRaynerDD

Left some nits

DarcyRaynerDD · 2021-09-23T18:46:26Z

README.md

+
+Inferred Spans are spans that Datadog can create based on incoming event metadata. 
+Set `DD_INFERRED_SPANS` to `true` to infer spans based on Lambda events.
+Inferring upstream spans is only supported if you are using the [Datadog Lambda Extension](https://docs.datadoghq.com/serverless/libraries_integrations/extension/). 


Is this actually true? I know we said we wouldn't do any specific work to make it function in the forwarder, but the span will still get generated. I thin originally I said the forwarder might remap the service name, but I don't know if we verified that.

We did verify it. The forwarder adds tags from the tags cache to the trace payloads it sends back. If we want, we can always fix this and add forwarder support later on.

hmm, i guess the forwarder applies the service tag on all the trace payloads from the same event object? if so, even the trace payload carrying the inferred span has no lambda function arn, it still assumes it's from the same function with other payloads and apply the tag? If yes, we would need to make some changes in the forwarder, hopefully a small one.

DarcyRaynerDD · 2021-09-23T18:57:18Z

datadog_lambda/tracing.py

+SPAN_TYPE_INFERRED = "inferred"
+
+
+class InferredSpanFilter(TraceFilter):


I think this warrants a comment explaining why we are using this.

DarcyRaynerDD · 2021-09-23T18:59:29Z

datadog_lambda/tracing.py

+    return None
+
+
+def detect_inferrable_span_type(event):


Is there a way we can plug this into the existing trigger detection logic. I'm wondering if we are duplicating the functionality somewhat.

You and Tian are really on the same brain wave here 😄

…type code

hghotra · 2021-10-27T15:09:53Z

datadog_lambda/trigger.py

+    Currently, only API Gateway events have subtypes, but I imagine we might see more in the
+    future.
+    This was added to support the difference in handling of e.g. HTTP-API and Websocket events vs
+    vanilla API-Gateway events.


👍🏼 This will be helpful in making a distinction in the future if we have to for any reason.

hghotra

💯

* Infer API Gateway spans (#172) * Infer spans from API Gateway events * Adding some prints. remove later * Change some info on the API Gateway span * Rename something * black * >:( * black * fix time * Support various API Gateway, HTTPAPI, and Websocket events * black * Add DD_INFERRED_SPANS env var to turn inferred spans on and off * infer spans in integration tests * specify which env var to set true in order to enable inferred spans * try setting inferred span name to inferred span URL * s/beta/experimental/ * Correctly create spans in separate services, assuming the extension is running and the tag is not set on the function * Remove function_name * Flush after closing spans * black * black * update snapshots * Make the snapshots valid json * black * Remove the inferredSpansFilter * Refactor inferred-span event type detection to use the trigger event type code * remove unused import * lines too long >=( * Finish refactor using _EventSource object * lol, remove println debugging * Update snapshots * Inferred spans get meta.span_type:inferred * [SLS-1594] Inferred spans for SQS & SNS (#190) * Simplify event source parsing logic * Add sns & sqs inferred spans * Fix default evt source bug & update comments * Use poetry for running unit tests * Add additional api gateway trigger tests * Fix bugs & refactor * Add inferred spans unit tests * Update integration tests * Run all tests for all python versions * Add missing change * Update _dd.span_type to span_type * Remove dependence on extension * Additional api gateway test * Remove log line & add todo * Fix api gateway integration test * Move call to get_first_record * Revert to using pip inside docker * [SLS-1671] Inferred Spans for Kineses, EventBridge, S3, DynamoDB (#191) * Add kinesis inferred span * Add dynamodb inferred span * Add s3 inferred span * Add eventbridge custom event inferred span * Comment out flaky units tests (TODO) * Add integration tests * Remove log line * Use timestamp instead of strftime (#194) * [SLS-1683] Add sync/async tag and set inferred span end time based on value (#195) * Add sync/async tag to inferred spans * End inferred span based on sync/async tag * Update tests * Fix issue with comparison * Fix small issues * Update integration test snapshots * [SLS-1713] Add tag for whether span should inherit service from lambda (#196) * Use service arg rather than service.name tag * Refactor inferred span metadata tags * black * Update inferred span keys * [SLS-1674] Additional attributes for different managed services (#200) * Additional attributes for sqs spans * Add additional attributes for sns * Add event subscription arn for sns * Add additional attributes for kinesis spans * Add additional attributes for dynamodb spans * Add additional attributes for s3 spans * Additional attribute for eventbridge spans * Add additional attributes for websocket spans * Fix websocket resource name * Additional attributes for apigateway v1 spans * Additional attributes for api-gateway v2 spans * Fix api-gateway v1 resource name * Fix merge issue * Add space to resource name * Update tests * black * Add typing_extensions to pyproject.toml * Update snapshots * Fix snapshots Co-authored-by: chris.agocs <chris.agocs@datadoghq.com> * [SLS-1824] SNS trace extractor (#201) * Extract trace context from sns payload * Fix extractor issue * Handle SNS message inside SQS event * Add extractor for Eventbridge context (#202) * Update expected breaking change date (#114) * change inferred_span to _inferred_span * Add extractor for eventbridge trace context * Add another test to test eventbridge extraction * Get tracing.Literal for pythons that don't have it already * Add _datadog to eventbridge extractor * Update integration tests * Remove init complete and main start logs Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com> Co-authored-by: Harvinder Ghotra <ghotra.harvinder@gmail.com> * Remove duplicate entry * Kinesis extractor plus small fixes * Add snapshots * Add tests Co-authored-by: Christopher Agocs <agocs@users.noreply.github.com> Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com> * Update stash * Address feedback * Default DD_TRACE_MANAGED_SERVICES to true Co-authored-by: Christopher Agocs <agocs@users.noreply.github.com> Co-authored-by: chris.agocs <chris.agocs@datadoghq.com> Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com>

chris.agocs added 13 commits July 26, 2021 17:20

Infer spans from API Gateway events

19f9da3

Adding some prints. remove later

c4cedfa

Change some info on the API Gateway span

2cfaee7

Merge branch 'main' into chris.agocs/inferring_api_gateway_spans_poc

e077667

Rename something

c1f11ee

black

0cd45fd

>:(

40c79ca

black

c4d751e

fix time

96b7844

Support various API Gateway, HTTPAPI, and Websocket events

586033d

black

7d91108

Add DD_INFERRED_SPANS env var to turn inferred spans on and off

2605f2e

infer spans in integration tests

4ecc91d

agocs marked this pull request as ready for review September 10, 2021 19:53

agocs requested a review from a team as a code owner September 10, 2021 19:53

agocs changed the title ~~Chris.agocs/inferring api gateway spans poc~~ Infer API Gateway spans Sep 10, 2021

chris.agocs added 2 commits September 10, 2021 16:13

specify which env var to set true in order to enable inferred spans

ceea1cc

try setting inferred span name to inferred span URL

f180439

Hesperide reviewed Sep 12, 2021

View reviewed changes

s/beta/experimental/

ef9439c

chris.agocs added 6 commits September 17, 2021 14:47

Correctly create spans in separate services, assuming the extension i…

07d2e97

…s running and the tag is not set on the function

Remove function_name

99ca466

Flush after closing spans

dd7840e

black

33274e3

black

8503277

update snapshots

279ffe4

chris.agocs added 3 commits September 22, 2021 11:43

Make the snapshots valid json

a580e45

merge integration tests

df376c9

black

3a3cc4e

tianchu reviewed Sep 23, 2021

View reviewed changes

DarcyRaynerDD approved these changes Sep 23, 2021

View reviewed changes

chris.agocs added 8 commits October 25, 2021 14:32

Remove the inferredSpansFilter

e277053

Refactor inferred-span event type detection to use the trigger event …

657ee36

…type code

remove unused import

a6170ff

lines too long >=(

e75a3a0

Merge main and blow away the integration test snapshots

5bd5cb5

Finish refactor using _EventSource object

38b6987

lol, remove println debugging

a1c0b9d

Update snapshots

b325ee7

agocs changed the base branch from main to inferred-spans-feature-branch October 26, 2021 19:23

hghotra reviewed Oct 27, 2021

View reviewed changes

hghotra approved these changes Oct 27, 2021

View reviewed changes

agocs merged commit c7440a5 into inferred-spans-feature-branch Oct 27, 2021

agocs deleted the chris.agocs/inferring_api_gateway_spans_poc branch October 27, 2021 15:18

		@@ -87,6 +87,16 @@ Initialize the Datadog tracer when set to `true`. Defaults to `false`.

		Set to `true` to merge the X-Ray trace and the Datadog trace, when using both the X-Ray and Datadog tracing. Defaults to `false`.

		### DD_INFERRED_SPANS (beta)

		SPAN_TYPE_INFERRED = "inferred"


		class InferredSpanFilter(TraceFilter):

Infer API Gateway spans #172

Infer API Gateway spans #172

Uh oh!

Conversation

agocs commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Known issue

Motivation

Testing Guidelines

Additional Notes

Types of Changes

Check all that apply

Uh oh!

agocs commented Sep 10, 2021

Uh oh!

Hesperide commented Sep 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianchu commented Sep 13, 2021

Uh oh!

agocs commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarcyRaynerDD left a comment

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!

hghotra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agocs commented Sep 7, 2021 •

edited

Loading

agocs commented Sep 14, 2021 •

edited

Loading