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

Add partial transaction spec for AWS lambda #799

Merged
merged 2 commits into from
May 24, 2023

Conversation

basepi
Copy link
Contributor

@basepi basepi commented May 23, 2023

I'm adding this spec after-the-fact, all relevant agents have already implemented this. Thus, I skipped the "draft" phase and went straight to final review.

This information is pulled from #753

@basepi basepi requested review from a team as code owners May 23, 2023 18:25
@basepi basepi removed request for a team May 23, 2023 18:26
request ID) and the Content-Type should be
`application/vnd.elastic.apm.transaction+ndjson`. If the extension returns
anything other than a 200 HTTP code, the agent should stop sending partial
transaction registrations for future invocations.
Copy link
Member

Choose a reason for hiding this comment

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

This should say the request is a POST. Also the "should"s for the headers could be changed to "must"s.

POST /register/transaction
Content-Type: application/vnd.elastic.apm.transaction+ndjson
x-elastic-aws-request-id: ${awsRequestId}

{"metadata":{...}}
{"transaction":{...partial transaction data...}}

the metadata, if possible.

Is there a case where that isn't possible?


Some other notes I had regarding the transaction json blob:

It should have a default outcome value. duration and result (and possibly outcome) fields will be set by the Elastic Lambda extension if this transaction is used.

Not sure if we want this spec to record this info.

Copy link
Contributor Author

@basepi basepi May 23, 2023

Choose a reason for hiding this comment

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

This should say the request is a POST. Also the "should"s for the headers could be changed to "must"s.

Updated.

Is there a case where that isn't possible?

I removed that language. Perhaps someone is collecting the metadata asynchronously in some way? I'll leave it as a SHOULD -- in theory if an agent isn't ready with the metadata they could send it to the normal endpoint and the extension would record it. Pretty sure it's optional in @lahsivjar's implementation.

Not sure if we want this spec to record this info.

Is there a default outcome value? I don't think it's important to record here, I didn't change anything about python's to_dict() function that encodes the transaction dictionary (except to remove side effects).

Copy link
Member

Choose a reason for hiding this comment

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

Is there a default outcome value?

You are right, this spec section shouldn't mention outcome. The Node.js APM agent does set a default transaction.outcome of 'unknown', but the intake spec allows transaction.outcome to be null -- i.e. it is optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure it's optional in @lahsivjar's implementation.

Yes, metadata is optional.

Copy link
Contributor

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

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

Thanks :) LGTM!

request ID) and the Content-Type should be
`application/vnd.elastic.apm.transaction+ndjson`. If the extension returns
anything other than a 200 HTTP code, the agent should stop sending partial
transaction registrations for future invocations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure it's optional in @lahsivjar's implementation.

Yes, metadata is optional.

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

LGTM

@gregkalapos gregkalapos self-requested a review May 24, 2023 09:29
@basepi basepi merged commit 9fadb84 into elastic:main May 24, 2023
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.

6 participants