feat: wire in AAP span processor in the trace_processor#767
Conversation
Annotates the spans in `TraceAgent::handle_traces` if there is a corresponding security context, and the span's name is `aws.lambda`. If the response for the request has not been seen yet, temporarily buffers the span to wait for completion before actually sending it out.
…dier/serverless-aap-port__span-processor
RomainMuller
changed the title
| } | ||
| } | ||
| impl Drop for Context { | ||
| fn drop(&mut self) { |
There was a problem hiding this comment.
Would this mean that there's a possibility the aws.lambda span will be completely dropped if appsec is not seen?
There was a problem hiding this comment.
Theoretically, yes... But all code paths that lead to dropping a Context from the buffer properly finalize it. This Drop here is additional assurance that this does not diverge in the future.
There was a problem hiding this comment.
Ok, sounds good, just wanted to check because if this happens, the invocation won't appear in the customers Serverless view, as we depend on the aws.lambda span
There was a problem hiding this comment.
Yes yes, I am pretty well aware of this... If everything wasn't async-tainted, I'd have released the trace in impl Drop but I can't, because I can't guarantee I have an async runtime available when Drop runs.
RomainMuller
force-pushed
the
romain.marcadier/serverless-aap-port__span-processor
branch
from
5f296a1 to
e1db1a0
Compare
…n (needs status code)
RomainMuller
force-pushed
the
romain.marcadier/serverless-aap-port__span-processor
branch
from
e1db1a0 to
d6ebaa6
Compare
| /// Whether we have had a chance to see the response body or not. | ||
| response_seen: bool, | ||
| /// Holds a trace for sending once the response is seen. | ||
| on_response_seen: Option<(Vec<Span>, SendingTraceProcessor, HoldArguments)>, |
There was a problem hiding this comment.
Should this instead be the something explaining which values it holds, and then we can use the existing method of is_pending_response as the boolean check?
That way, when we are doing take() its easier to read?
| } | ||
|
|
||
| /// Marks the response of this request as having been seen. | ||
| pub(super) async fn set_response_seen(&mut self) { |
There was a problem hiding this comment.
the name set_response_seen is confusing because hold_trace is the one who actually sets the response as seen 🤔 whereas in here we are just processing + sending data to the trace aggregator
There was a problem hiding this comment.
I see the confusion here... We mark the Context as having seen the response, which possibly allows a trace to be flushed if one was held; or will cause the trace to not be held if we haven't seen it yet.
I was thinking about possibly renaming this to something like finalize so it's context.finalize()... I'd then add some debug_assert! in other setters to ensure we don't try to re-use a finalized context.
| response_seen: false, | ||
| on_response_seen: None, |
There was a problem hiding this comment.
Oh yeah having response_seen and on_response_seen confused me a lot, I just saw these are two different things
There was a problem hiding this comment.
It's a "classic" finalized/onFinalize pair. But we can model this a little more to make it more obvious.
duncanista
left a comment
duncanista
left a comment
There was a problem hiding this comment.
Overall looks good, left some comments with suggestions and questions!
Co-authored-by: jordan gonzález <30836115+duncanista@users.noreply.github.com>
RomainMuller
force-pushed
the
romain.marcadier/serverless-aap-port
branch
from
3a9bf52 to
96ffde8
Compare
RomainMuller
force-pushed
the
romain.marcadier/serverless-aap-port__span-processor
branch
from
47259ca to
466de2c
Compare
duncanista
force-pushed
the
romain.marcadier/serverless-aap-port__span-processor
branch
from
c924410 to
a54a1d1
Compare
There was a problem hiding this comment.
Size differs from the one in here 1df2130
Are they different?
There was a problem hiding this comment.
Ah yes it's quite possible git resolved the conflict the wrong way... lemme make sure this is latest.
…er/serverless-aap-port__span-processor
RomainMuller
merged commit acde692
into
romain.marcadier/serverless-aap-port
RomainMuller
deleted the
romain.marcadier/serverless-aap-port__span-processor
branch
2 participants
Annotates the spans in
TraceProcessor::process_tracesif there is a corresponding security context, and the span's name isaws.lambda.If the response for the request has not been seen yet, temporarily buffers the span to wait for completion before actually sending it out.