-
Notifications
You must be signed in to change notification settings - Fork 28
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
Allow metadata in register transaction #384
Conversation
This is slightly concerning to me. We have a mechanism for adding "late-breaking" metadata in the python agent, for metadata that is only available at request time. For now, this should not be a problem -- the only late metadata we add is for lambda, and that metadata will be available when we send the partial transaction. I just wanted to call it out as something we should explicitly state when we update the spec with partial transaction support. |
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.
👍 to stated changes, haven't reviewed the code.
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.
The functionality sounds good. I won't have a chance to test this for a little while yet (hopefully this week).
) | ||
|
||
const txnRegistrationContentType = "application/vnd.elastic.apm.transaction+json" | ||
const txnRegistrationContentType = "application/vnd.elastic.apm.transaction+ndjson" |
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.
is it OK to break backwards compatibility here? Is there a concern for using older agents? AFAICS, if older agents attempt to send data to this endpoint, we'll just reject it and lose the partial transaction information.
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.
This hasn't been implemented yet by any agent, in fact python agent is the first one to start implementing it so backward compatibility is not an issue at this point.
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.
Code LGTM
@lahsivjar I have not yet, but I think I may start looking at this tomorrow (as part of elastic/apm-agent-nodejs#3136). |
I'm working on testing this today. I was putting it off because I haven't gotten around to building the extension from source yet. 😅 |
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.
Tested, working great!
The PR updates the
/register/transaction
endpoint to acceptNDJSON
. The/register/transaction
endpoint can now acceptmetadata
as the first event inNDJSON
body and cache it for future calls. A complete list of changes introduced in the PR is as follows:Content-Type
for the/register/transaction
endpoint fromapplication/vnd.elastic.apm.transaction+json
toapplication/vnd.elastic.apm.transaction+ndjson
.deflate
andgzip
content encoding for the/register/transaction
endpoint.metadata
as the first event in theNDJSON
body to the/register/transaction
endpoint. Themetadata
is cached and used to send data to APM-Server. The cached metadata is NOT updated by the metadata sent via the intake endpoint.Related Issue
Closes #352