-
Notifications
You must be signed in to change notification settings - Fork 35
Handle txn registration if invocation registration is late #344
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one question.
accumulator/batch.go
Outdated
if !isTransactionEvent(payload) { | ||
return errors.New("invalid payload") | ||
// It is possible that the invocation is registered at a later time | ||
b.invocations[waitingInvocationKey] = &Invocation{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we require the agent to send the Lambda request ID along with the transaction ID and payload? Then it can always match up on request ID, and remove currentlyExecutingRequestID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds plausible. I was motivated by keeping the lifecycle simple by exposing less arguments to the agent but I think sending requestID (maybe as a header?) will be better. Let me make the adjustments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the PR to accept request ID as a header x-elastic-aws-request-id
. I didn't remove currentlyExecutingRequestID
because I think adding the constraint for all agent data to have request ID as a header might not be a good idea but adding the requirement to the txn registration API should be okay. Additionally, the currentlyExecutingRequestID
can be set reliably in both agent init or register invocation stages to ensure a correct value for later calls. Let me know your thoughts on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I forgot that we still need to associate events with the invocation. Looks good, thanks.
What the PR does?
runtimeDone
log timestamp for agent issues.Related to elastic/apm-agent-go#1323