Skip to content
This repository was archived by the owner on Aug 4, 2023. It is now read-only.

Conversation

trentm
Copy link
Member

@trentm trentm commented Feb 24, 2022

This adds 'Client#lambdaStart()' and the 'lambdaEnd: true' option to
'Client#flush([opts,] [cb])' to support for flushing Lambda invocation
tracing data and signaling the Elastic Lambda extension per spec.
https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-aws-lambda.md#data-flushing

Refs: elastic/apm-agent-nodejs#2485

Checklist

  • waiting for extension panic fixes before enabling the '?flushed=true' signaling by default
  • tests of the new APIs

@trentm trentm self-assigned this Feb 24, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Feb 24, 2022
@ghost
Copy link

ghost commented Feb 24, 2022

💚 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: 2022-03-04T00:58:23.109+0000

  • Duration: 9 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 287
Skipped 0
Total 287

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…t' due to this socket unref

Given that we now ensure the intake request and flush completes before
calling the Lambda handler callback, we *want* to keep that socket ref'd.
This fixes an issue with corking when events come after a
client.flush({lambdaEnd: true}) in *the same tick*. Before this those
events could slip into the stream and result in a subsequent intake
request starting after the `lambdaEnd`... which would often result in
those events being lost.
@trentm trentm marked this pull request as ready for review March 4, 2022 00:54
@trentm trentm requested a review from astorm March 4, 2022 00:54
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

I don't really have a good mental model for how this client, particularly the flushing, works anymore so I can't really give a canonical 👍 or 👎 on what overall effect this new code will have.

However -- I've reviewed the new branching and from what I see the new Lambda flushing features appear to be gated correctly and leaves the non-lambda functionality intact. In the few places where the mainline functionality has been altered (ex. addition of the flush marker to _writeFlush) the changes are minor enough that it shouldn't effect the overall behavior of the client when operating in non-lambda mode.

Also, for what it's worth, this is another example where a separate/different HTTP client (that implements the same defacto-public interface) for Lambda would make this sort of functionality less-risky/less-difficult to implement. It may make sense in the future to take advantage of the Agent's architecture around it's transport object. Not recommending this change be made now, just dinging that bell again.

Approving.

@trentm
Copy link
Member Author

trentm commented Mar 9, 2022

[email protected] is published

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants