Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add tracestate header when starting lambda transaction #2185

Merged
merged 6 commits into from
Aug 11, 2021

Conversation

astorm
Copy link
Contributor

@astorm astorm commented Aug 9, 2021

Checklist

Part of #2156

This PR ensures that if the Lambda function is responding to an API Gateway request (or other request that includes a tracestate header) that its transaction will start using that tracestate. This is needed because our existing lambda instrumentation was written before tracestate was a thing.

  • Implement code
  • Add tests

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Aug 9, 2021
@astorm astorm marked this pull request as draft August 9, 2021 20:43
@apmmachine
Copy link
Contributor

apmmachine commented Aug 9, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-10T21:16:41.329+0000

  • Duration: 19 min 49 sec

  • Commit: 5f8c41e

Test stats 🧪

Test Results
Failed 0
Passed 20
Skipped 0
Total 20

Trends 🧪

Image of Build Times

Image of Tests

@astorm astorm self-assigned this Aug 9, 2021
@astorm astorm marked this pull request as ready for review August 9, 2021 21:40
@astorm astorm requested a review from trentm August 9, 2021 21:41
lib/lambda.js Outdated
@@ -86,11 +87,14 @@ module.exports = function elasticApmAwsLambda (agent) {
* elastic-apm-traceparent if available.
*/
parentId = value
Copy link
Member

Choose a reason for hiding this comment

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

  1. The handling will have to change to avoid that break preferring 'elastic-apm-traceparent' to 'traceparent'. IIUC, with the current logic it will miss tracestate if it comes after 'elastic-apm-traceparent' in payload.headers.
  2. Also, per feat: prefer 'traceparent' header over 'elastic-apm-traceparent' header #2079 the logic should change (could be a separate PR) to prefer 'traceparent' to 'elastic-apm-traceparent'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trentm Ugh -- that's a weird construct. Thanks for catching it. Any strong preference between

  • more code and fewer loop cycles (matching most closely with current intent/pattern)
    if (payload.headers !== undefined) {
      for (const [key, value] of Object.entries(payload.headers)) {
        const lowerCaseKey = key.toLowerCase()
        if (lowerCaseKey === 'traceparent') {
          parentId = value
        } else if (!parentId && lowerCaseKey === 'elastic-apm-traceparent') {
          parentId = value
        } else if(lowerCaseKey === 'tracestate') {
          tracestate = value
        }
        if(parentId && tracestate) {
          break
        }
      }
    }
  • or less code but slightly more work normalizing all the headers
      if (payload.headers !== undefined) {
        const normalizedHeaders = {}
        for (const [key, value] of Object.entries(payload.headers)) {
          const lowerCaseKey = key.toLowerCase()
          normalizedHeaders[lowerCaseKey] = value
        }
        parentId = normalizedHeaders.traceparent ? normalizedHeaders.traceparent : normalizedHeaders['elastic-apm-traceparent']
        tracestate = normalizedHeaders.tracestate
      }

Copy link
Member

Choose a reason for hiding this comment

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

The former will prefer 'elastic-apm-traceparent' over 'traceparent' if it and 'tracestate' come first, so doesn't work as written.

No strong preference between those two until perf profiles show that the section is a hot spot.

Slightly related: from old https://elastic.slack.com/archives/C5M02D1GE/p1619802623086700?thread_ts=1619742610.083400&cid=C5M02D1GE for ([key, value] of Object.entries(obj)) is dog slow. Perhaps premature optimization, but I'd generally bias against using it.

@astorm astorm changed the title feat: add tracestate header when starting transaction feat: add tracestate header when starting lambda transaction Aug 10, 2021
@astorm
Copy link
Contributor Author

astorm commented Aug 10, 2021

I've replaced the Object.entries() loop with an Object.keys() loop. Always up for quashing these then we see them and remember.

I've opted for the clearer (to me) code of normalizing all the headers rather than the micro-optimization of finishing once we have the specific headers we're interested in.

I've added test cases with both elastic-apm-traceparent by itself and elastic-apm-traceparent both before and after the traceparent header to ensure that we prefer traceparent when it's there, but still pickup elastic-apm-traceparent when it's by itself.

@astorm astorm requested a review from trentm August 10, 2021 21:42
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks.

@astorm astorm merged commit 4a57855 into master Aug 11, 2021
@astorm astorm deleted the astorm/tracestate-lambda branch August 11, 2021 16:37
dgieselaar pushed a commit to dgieselaar/apm-agent-nodejs that referenced this pull request Sep 10, 2021
…#2185)

* feat: add tracestate header when starting a lambda transaction

part of elastic#2156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants