Skip to content

Update trace propagation format for aws-sdk direct invocations - #1506

Merged
rochdev merged 26 commits into
masterfrom
chris.agocs/update_lambda_direct_invocation_trace_propagation_format
Aug 11, 2021
Merged

rochdev merged 26 commits into
masterfrom
chris.agocs/update_lambda_direct_invocation_trace_propagation_format

Conversation

@agocs

@agocs agocs commented Jul 16, 2021 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Updates the trace propagation format for aws-sdk direct lambda invocations. The old way was:

clientContext: {
  custom:{
    _datadog: {
      x-datadog-trace-id: "",
      x-datadog-parent-id: "",
      ...
    }
  }
}

New way is:

clientContext: {
  custom: {
      x-datadog-trace-id: "",
      x-datadog-parent-id: "",
      ...
  }
}

I've added a configuration option called invokeWithLegacyContext (default false) that causes aws-sdk.invoke operations to use the legacy trace propagagtion format. I'm not sure where to document the versions of datadog-lambda-py and datadog-lambda-js that are compatible with the new format.

Reasoning

The Go and Java runtimes only support ClientContext.custom fields of type map[string]string. C.f. :

In order to pass trace contexts to directly invoked Go and Java lambda functions, we can write the trace context key:values to ClientContext.custom rather than creating a ClientContext.custom._datadog object like we've been doing.

Backwards compatibility

This is, strictly speaking, a breaking change in how we add trace context to direct invocations. The Python and JS repos have already been updated to accept the new format:

They both check for the old and new formats.

Plugin Checklist

Additional Notes

@codecov

codecov Bot commented Jul 28, 2021 •

Copy link
Copy Markdown

Codecov Report

Merging #1506 (5b2f63c) into master (ae2cc0b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1506      +/-   ##
==========================================
- Coverage   94.18%   94.17%   -0.01%     
==========================================
  Files         157      157              
  Lines        6409     6406       -3     
==========================================
- Hits         6036     6033       -3     
  Misses        373      373              
Impacted Files Coverage Δ
packages/datadog-plugin-aws-sdk/src/index.js 100.00% <ø> (ø)
...ages/datadog-plugin-aws-sdk/src/services/lambda.js 90.90% <100.00%> (-1.10%) ⬇️

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 ae2cc0b...5b2f63c. Read the comment docs.

@agocs
agocs marked this pull request as ready for review July 28, 2021 20:41
@agocs
agocs requested a review from a team as a code owner July 28, 2021 20:41

@bengl bengl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, but I think it might be semver-major. Maybe we can default to the "legacy" way now, and save changing the default for an upcoming major rev?

@rochdev what do you think?

@agocs

agocs commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Hey @bengl , I flipped invokeWithLegacyContext such that it defaults true. Once this is merged, should I queue up another PR to flip it back, tagged with the 2.0.0 milestone?

@agocs

agocs commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Per our conversation on Slack, I've pulled out the invokeWithLegacyContext option because the old way can crash Java and Go lambda functions, but the worst that's going to happen the new way is that an older version of datadog-lambda-python or datadog-lambda-js won't be able to find the trace context.

@agocs

agocs commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Hm, it looks like Codecov doesn't like that I decreased the overall line count. Is this a blocker?

@rochdev

rochdev commented Aug 6, 2021

Copy link
Copy Markdown
Member

Hm, it looks like Codecov doesn't like that I decreased the overall line count. Is this a blocker?

Not a blocker.

@rochdev
rochdev merged commit 0425014 into master Aug 11, 2021
bengl pushed a commit that referenced this pull request Aug 16, 2021
* refactor requestInject (should be a noop). struggling to get docker-compose up to work on my laptop

* Revert "refactor requestInject (should be a noop). struggling to get docker-compose up to work on my laptop"

This reverts commit 41d289c.

* refactor requestInject (should be a noop)

* sneakily add in a couple console.logs for circleci

* sneakily add in a few more console.logs for circleci

* console.log any caught error

* getting closer...

* request.params.clientContex -> ClientContext

* remove console.logs

* only set trace context in one place

* inject trace context directly into clientContext.custom._datadog (without first instantiating)

* initiailize clientContext.custom._datadog before injecting context

* console.log the request to see if it already has a config

* console.log the request to see if it already has a config

* Document the new parameter

* Attempt to add invokeWithLegacyContext config parameter

* oops added a second .

* Move the console.log so I can see the error that was gonna get caught

* Pass config into requestInject

* Remove setConfig()

* Add tests for invokeWithLegacyContext = false

* Remove extra console.log

* Add tests for default config

* Flip the default value of invokeWithLegacyContext

* Remove invokeWithLegacyContext option
bengl pushed a commit that referenced this pull request Aug 16, 2021
* refactor requestInject (should be a noop). struggling to get docker-compose up to work on my laptop

* Revert "refactor requestInject (should be a noop). struggling to get docker-compose up to work on my laptop"

This reverts commit 41d289c.

* refactor requestInject (should be a noop)

* sneakily add in a couple console.logs for circleci

* sneakily add in a few more console.logs for circleci

* console.log any caught error

* getting closer...

* request.params.clientContex -> ClientContext

* remove console.logs

* only set trace context in one place

* inject trace context directly into clientContext.custom._datadog (without first instantiating)

* initiailize clientContext.custom._datadog before injecting context

* console.log the request to see if it already has a config

* console.log the request to see if it already has a config

* Document the new parameter

* Attempt to add invokeWithLegacyContext config parameter

* oops added a second .

* Move the console.log so I can see the error that was gonna get caught

* Pass config into requestInject

* Remove setConfig()

* Add tests for invokeWithLegacyContext = false

* Remove extra console.log

* Add tests for default config

* Flip the default value of invokeWithLegacyContext

* Remove invokeWithLegacyContext option
@tlhunter
tlhunter deleted the chris.agocs/update_lambda_direct_invocation_trace_propagation_format branch January 19, 2024 22:18
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.

3 participants