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

RFC: instrument lambda handler #162

Closed
wants to merge 16 commits into from

Conversation

pokryfka
Copy link
Contributor

PoC instrumentation of the lambda handler.

Motivation:

Add support for instrumentation.

Modifications:

  • creates an instance of a tracer (currently AWSXRayRecorder) - needs to be configurable
  • pass tracer TracingInstrument and (context) baggage BaggageContext in Lambda.Context
  • instrument invocation handling
  • flushes the tracer after each invocation

TODO

  • import swift-tracing instead of aws-xray-sdk-swift to limit dependencies
    • stable TracingInstrument API
  • extend Lambda bootstrap to be able to use the same EventLoop in an instance of XRayUDPEmitter
  • discuss what exactly should be recorder by the handler

Result:

Example in X-Ray console of handling a simple HelloWorld, cold start, Foundation JSON encoder/decoder.

Screen Shot 2020-08-14 at 11 08 02

@swift-server-bot
Copy link

Can one of the admins verify this patch?

3 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@pokryfka
Copy link
Contributor Author

CC @ktoso @slashmo

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2020

@swift-server-bot add to whitelist

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2020

Awesome 👏 😎 Will give it a look :-)

@pokryfka
Copy link
Contributor Author

@ktoso

Awesome 👏 😎 Will give it a look :-)

I have been using it for a week, obviously its PoC state, but I think PR may be a good way to discuss how it could be integrated including context propagation (note that Lambda defines its own Context which I used to pass the "context baggage" BaggageContext hmm...)

Attributes aside, I think it would be great to be able to share EventLoop to send UDP;
something XRayRecorder does support but is currently not exposed by Lambda, InstrumentationSystem (bootstrap) and TracingInstrument (forceFlush)

@pokryfka
Copy link
Contributor Author

14:23:44 Precondition failed: client not stopped before the deinit.: file /code/.build/checkouts/aws-xray-sdk-swift/Sources/AWSXRayUDPEmitter/UDPClient.swift, line 71

this breaks the tests at the moment, I was hoping ServiceLifecycle support will get merged #141

@slashmo
Copy link

slashmo commented Aug 14, 2020

Nice @pokryfka, thanks for taking the initiative! 🎉

@tomerd tomerd added the discussion for things that need discussion label Aug 14, 2020
@tomerd tomerd changed the title WIP: instrument lambda handler RFC: instrument lambda handler Aug 14, 2020
@tomerd tomerd marked this pull request as draft August 14, 2020 18:16
@tomerd
Copy link
Contributor

tomerd commented Aug 14, 2020

thanks @pokryfka tagged this as RFC/draft for discussion

@@ -106,7 +116,12 @@ extension Lambda {
var logger = logger
logger[metadataKey: "awsRequestID"] = .string(requestID)
logger[metadataKey: "awsTraceID"] = .string(traceID)
var baggage = BaggageContext()
// TODO: handle error
baggage.xRayContext = try? XRayContext(tracingHeader: traceID)
Copy link
Contributor

@tomerd tomerd Aug 14, 2020

Choose a reason for hiding this comment

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

would we want to let the users decide what tracer to use, or since this is AWS oriented anyways just pin to x-ray?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that's the main TODO in this PR - not using the xray explicitly.

Even if it is bound to xRay in reality, we should make sure to only use the abstract API, maybe maybe some day there would be some other tracer or maybe amazon decide to make their own or something, no idea, but let's keep the door open for future evolution.

Using the tracing API also means that while developing locally you could plug in the Instruments(.app) (naming gets confusing...) tracer: slashmo/gsoc-swift-tracing#97 and see spans in Instruments on the mac. Instruments does not really understand / visualize "traces" with parents etc well today... but it's something we can keep in mind, maybe it'll get better at displaying those and then when developing locally you get the same user experience with tracing as on prod :-)

Copy link
Member

Choose a reason for hiding this comment

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

@ktoso I generally agree with you. I'm however a little concerned, that tracing will require manual adjustment (incl. adding another dependency) if we don't include the XRay tracing by default. Lambda tracing wouldn't work out of the box in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I get that -- though the lambda runtime "core" should not be instrumented using any specific tracer, regardless of "yes it'll be xray" but maybe some day down the road there's other impls, and you'd want to swap it.

We could absolutely though make some "batteries included" package, we should think how to pull that off, wdyt?

}
}.flatMap {
// flush the tracer after each invocation
self.tracer.flush(on: self.eventLoop)
Copy link
Contributor

Choose a reason for hiding this comment

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

should trace deeper in this method, e.g. the getNextInvocation call?

Copy link
Contributor

Choose a reason for hiding this comment

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

would this be always instead of flatMap?

Copy link
Contributor Author

@pokryfka pokryfka Aug 17, 2020

Choose a reason for hiding this comment

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

would this be always instead of flatMap?

correct, it should always flush (also when there were errors);
fixed and created a NIO convenience method flush to make it easier in other places

}
}.flatMap { invocation, result in
}.flatMap { (invocation, result, baggage: BaggageContext) in
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd pass the context not just baggage. seems cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BaggageContext is value type, Lambda.Context is reference type

here we want to pass the "original" baggage value, while the value of context.baggage may have been changed by the time we get the handler result

maybe Lambda.Context should also be a struct or perhaps BaggageContext should be propagated separately?

@ktoso WDYT?

@ktoso
Copy link
Contributor

ktoso commented Aug 15, 2020

(note that Lambda defines its own Context which I used to pass the "context baggage" BaggageContext hmm...)

Yeah, that's as-intended 👍

The lambda context should conform to LoggingBaggageContextCarrier as well (open to shorter /better names btw, could not think of a better one so far for this protocol 🤔 ).

switch self.decodeIn(buffer: event) {
let segment = context.tracer.beginSegment(name: "HandleEvent", baggage: context.baggage)
// TODO: record errors propagated in result types?
let decodedEvent = segment.subsegment(name: "DecodeIn") { _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I've been wondering if we should offer tracer.withSpan() { ... } as built-in for wrapping synchronous blocks with a span in the swift-tracing API right away, or if we should stay away from adding any kind of sugar in the API package.

This is quite common I think so I think we could add it...

We could also do the same with a NIO extensions package then to handle Future returning blocks 🤔

WDYT @pokryfka @tomerd ?

Copy link
Member

Choose a reason for hiding this comment

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

@ktoso +1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do use "helpers" both with closures and NIO futures as referenced here slashmo/gsoc-swift-tracing#125 (comment)
when API is changed to TracingInstrument the amount of sugar will depend on what swift-tracing provides

throw CodecError.responseEncoding(error)
case .success(let buffer):
return buffer
// TODO: use NIO helpers?
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomerd packaging wise, I'm thinking we should separate out all such support packages from the swift-tracing repo... like swift-tracing-support and there we'd add all such wrappers, also for URLSession and similar...? WDYT about separating it entirely on a repo level (not just separate module), so those can be versioned separately -- I suspect there can be much more "churn" on the "sugar" APIs than on the core stable boring API after all.

subsegment.end()
}
.flatMapThrowing { out in
try context.tracer.segment(name: "EncodeOut", baggage: segment.baggage) { _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming nitpick: I really would like to get all libs to be consistent with the use of the context parameter name, note that it can then accept a carrier and it becomes easier to just pass the context: segment / context: span / context: context (the lambda context); the only exception is context.baggage(like inLambda.Context`)

Copy link
Contributor Author

@pokryfka pokryfka Aug 17, 2020

Choose a reason for hiding this comment

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

this will be "context" when using TracingInstrument API:

var span = tracer.startSpan(named: "EncodeOut", context: segment.baggage) 

I dont have strong opinion on that, but to avoid confusion in XRaySDK I use "context" for XRayContext type (which does have the X-Ray trace context, strongly typed) and "baggage" for BaggageContext which may have the X-Ray trace context

var baggage = BaggageContext() // empty
let context = XRayContext()
baggage.xRayContext =  context

let segment = tracer.beginSegment(name: "EncodeOut", baggage: baggage) // may report missing context
// or
let segment2 = tracer.beginSegment(name: "EncodeOut", context: context)

Copy link
Contributor

@ktoso ktoso Aug 17, 2020

Choose a reason for hiding this comment

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

What would you say about the following spelling though:

var context = BaggageContext()
context.xRay =  XRayContext()
// since also:
// context.zipkin = ZipkinState() // I call it state in some things I worked on,
//
// because it aligns with  https://www.w3.org/TR/trace-context/#combined-header-value
// "tracestate" where each tracer may carry their own state by a vendor identified key
//
// Example: vendorname1=opaqueValue1,vendorname2=opaqueValue2

also because one can use the BaggageContextCarrier to assign through into the underlying baggage context;
This way all use sites, regardless if a carrier, raw baggage context, or any "my specific framework type" can use the same call site style:

func x(context: SomeFramework) { context.xRay ...
func x(context: BaggageContext) { context.xRay ...
func x(context: BaggageContextCarrier) { context.xRay ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the point with

let segment = tracer.beginSegment(name: "EncodeOut", baggage: baggage)
// or
let segment2 = tracer.beginSegment(name: "EncodeOut", context: context)

though; but that's the segment API, you are free to do what you want there but still I would not recommend using baggage as parameter names, you'd want to accept a BaggageCarrierCarrier most likely, and I would suggest calling it context for consistency -- people don't need to overthink it. But that's your segment API - you're free to design that how you want, but just a suggestion to keep in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a ticket to discuss naming once more: slashmo/gsoc-swift-baggage-context#23

import Dispatch
import Logging
import NIO

public typealias TracingInstrument = XRayRecorder
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, that confused me for a moment why it's an instrument but used segment() ;)

Okey for the PoC, agreed on migrating once "released" though it would be good to use this PR to already migrate and see that the API supports everything one needs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should make the API transition easier, temporary ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to PoC this out using the real swift-tracing API right away though?
We need real feedback on the API by using it in real use-cases, such as instrumenting lambda, http client and grpc; If there's things that show up as missing or sucky during that work now is the time to keep fixing the API. We should not wait for the API to move to its final destination to PoC things out and provide feedback. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be pretty much straight forward as far API is concerned.

I think the missing part is to freeze and release TracingInstrument API.
it will not be the final release but, if you consider existing API complete or a candidate for that, lets make a release and try to use it.

i am trying to keep XRayInstrument in sync but theres been many breaking changes recently in baggage and swift-tracing; I pin versions using git hashes, quite randomly.

@@ -28,6 +33,7 @@ extension Lambda {
init(eventLoop: EventLoop, configuration: Configuration) {
self.eventLoop = eventLoop
self.runtimeClient = RuntimeClient(eventLoop: self.eventLoop, configuration: configuration.runtimeEngine)
self.tracer = XRayRecorder(eventLoopGroupProvider: .shared(eventLoop))
Copy link
Contributor

Choose a reason for hiding this comment

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

I see... wanting to share the EL can be good... We'll need to figure out how to allow this when using the abstract API. @tomerd I guess users don't really create the EL, the framework creates it right?

Since the tracing API can't depend on NIO, we can't solve it by initializers in there; we could solve it perhaps by lambda offering some bootstrapTracer: EL -> TracingInstrument? Or users have to create the event loop (group), then bootstrap their tracer, then run the lambda... a bit more ceremony I guess hm. Need to look into this more

Copy link
Member

Choose a reason for hiding this comment

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

I see... wanting to share the EL can be good... We'll need to figure out how to allow this when using the abstract API. @tomerd I guess users don't really create the EL, the framework creates it right?

YES

Since the tracing API can't depend on NIO, we can't solve it by initializers in there; we could solve it perhaps by lambda offering some bootstrapTracer: EL -> TracingInstrument?

+1

Or users have to create the event loop (group), then bootstrap their tracer, then run the lambda... a bit more ceremony I guess hm.

-1

Copy link
Contributor Author

@pokryfka pokryfka Aug 17, 2020

Choose a reason for hiding this comment

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

@fabianfett I would really appreciate any suggestions how/where to configure/bootstrap tracer.

The challenge here is to pass the event loop which is currently not exposed by InstrumentationSystem nor Lambda runtime.

XRayRecorder can create its own but its not optimal.

let instrument = XRayRecorder() // same as XRayRecorder(eventLoopGroupProvider: .createNew)
defer { instrument.shutdown() }
InstrumentationSystem.bootstrap(instrument)

I did make quick PoC of bootstrap, sth like:

    internal typealias TracerFactory = (EventLoop) -> TracingInstrument
    private static var tracerFactory: TracerFactory = { _ in NoOpTracingInstrument() }

    public static func bootstrap(_ tracerFactory: @escaping (EventLoop) -> TracingInstrument) {
        self.lock.withLockVoid {
            self.tracerFactory = tracerFactory
        }
    }

the problem is, however, that that way Lambda runtime would also need to shutdown the tracer later on

this will most likely be affected (get easier?) by ServiceLifecycle integration #141

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Did a first review pass, looking good overall 👍

@@ -117,9 +127,9 @@ extension Lambda {
logger[metadataKey: "awsRequestID"] = .string(requestID)
logger[metadataKey: "awsTraceID"] = .string(traceID)
var baggage = BaggageContext()
// TODO: handle error
// TODO: use `swift-tracing` API, note that, regardless, we can ONLY extract X-Ray Context
Copy link
Contributor

Choose a reason for hiding this comment

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

not really; "we" don't decide what we extract, the configuration of instruments decides that, and yes since xray would be configured it'd extract it's own context here.

Specifically:

tracer.extract(<from where to extract, could be a dictionary> / http headers etc, into: &baggage, using: extractor apropriate to the first parameter, so http headers or similar)

Copy link
Contributor Author

@pokryfka pokryfka Aug 17, 2020

Choose a reason for hiding this comment

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

XRayInstrument can use the Instrument.extract API (its implemented and tested)

the problem here is that only the X-Ray trace context is provided by Lambda Runtime API (in header), see https://docs.aws.amazon.com/lambda/latest/dg/runtimes-api.html#runtimes-api-next

Context for other instruments may be provided in invocation payload and needs to be extracted by user in lambda event handler they implement.
(Note that AWSLambdaRuntimeCore only requires and knows that events, provided in invocation payload, are Decodable).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I missed that bit of the Lambda design. So in this integration style, even if one triggered the lambda via an http request, it would not get “the http request” but just the body, and the headers are the ones as listed on there, including the XRay trace header etc.

There AFAIR exists an integration mode though to get the entire request, right?

Pass through the entire request – A Lambda function can receive the entire HTTP request (instead of just the request body) and set the HTTP response (instead of just the response body) using the AWS_PROXY integration type.

Is this something that the runtime currently is able to handle? Seems more to be about how the API Gateway is configured right? Though I’ve not had the time to dig deeper into this yet.

You’d probably know more about this @fabianfett, we can catch up about this today maybe?

Copy link
Contributor Author

@pokryfka pokryfka Aug 18, 2020

Choose a reason for hiding this comment

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

I guess you are referring to API Gateway Lambda proxy integration

This does NOT affect Lambda custom runtime API, it affects API Gateway v1 "REST API" routing which, if configured that way, does not try to route events based on a RESTful model, instead it forwards all events to lambda which needs to resolve HTTP method, path and arguments itself based on the content in the event payload (but it still remains in the event payload -> invocation payload):

ANY /{proxy+}: The client must choose a particular HTTP method, must set a particular resource path hierarchy, and can set any headers, query string parameters, and applicable payload to pass the data as input to the integrated Lambda function.

Note that:

  • events with HTTP requests created by API Gateway may have different syntax, specifically:
  • event types are not defined in AWSLambdaCore but in AWSLambdaEvents which, unlike AWSLambdaCore, does have dependency on Foundation
  • user can choose to define its own In and Out types as long as they are Decodable/Encodable (I do it that way).
  • AWS does not limit context propagation to API Gateway (but it will not propagate context for other instruments; it MAY copy headers from the original requests and includes them in event payload, which is true for API Gateway)

For reference Integrating AWS X-Ray with other AWS services

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining this in more depth @pokryfka, I need to read up some more here about aws/lambda in general it seems.

@@ -52,6 +53,9 @@ extension Lambda {
/// Lambda runtime context.
/// The Lambda runtime generates and passes the `Context` to the Lambda handler as an argument.
public final class Context: CustomDebugStringConvertible {
// TODO: use RWLock (separate PR)
private let lock = Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh we can start out with just a lock and change only if proven to matter a lot;

The https://blog.nelhage.com/post/rwlock-contention/ keeps being brought up when we reach for RWLocks recently

Copy link
Contributor

Choose a reason for hiding this comment

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

RWlock are great when you do single (or very very very little) write and all-reads. in mixed mode it can get tricky tp get good performance

@@ -130,6 +135,14 @@ public enum Lambda {
}
}

// TODO: this is broken, the tracer tries to eat cake and have cake: flash on eventLoop then syncShutdown
// fix in XRayRecorder
tracer.shutdown { error in
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move this to L129

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomerd

the problem is that the current implementation in tracer.shutdown:

  • tries to flush first (on created or shared EventLoop),
  • waits for it to finish,
  • then shutdowns UDP client (which shares or owns the EventLoop).

this does not work well if lifecycle and tracer share the EventLoop:

Precondition failed: BUG DETECTED: wait() must not be called when on an EventLoop.

Not sure what the best solution is, I am thinking to remove flushing operation from shutdown,
that way client could always flush on the loop and then shutdown would just close all resources without trying to make any operation on the EventLoop.

@ktoso TracingInstrument defined in swift-tracing only defines interface to sync flush.

public protocol TracingInstrument: Instrument {
  func forceFlush()
  // ...
}

for reference types in AWSXRayRecorder

class XRayRecorder {
  // tries to flush firs
  public func shutdown(_ callback: ((Error?) -> Void)? = nil) { }
 // sync flush 
  public func wait(_ callback: ((Error?) -> Void)? = nil)
}

extension XRayRecorder {
    public func flush(on eventLoop: EventLoop) -> EventLoopFuture<Void> {}
}

Copy link
Contributor

@ktoso ktoso Aug 21, 2020

Choose a reason for hiding this comment

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

This is somewhat of a layering question.

swift-tracing cannot know about nio, so nothing about ELs or similar.

We could expose a shutdown(callback) if needed; question being, should it also flush, does flush need a callback as well then? Flushing is best-effort today and simply a signal to the tracer to try to flush.

The tracing instruments do not define shutdown because, similar like metrics, it is kind of assumed you're managing its lifecycle. We thought that normally you'd likely hook into swift-lifecycle with your tracer, and that'd be a specifc type there so you can do whatever you want.

Open to ideas here, what should we co in the API layer to help?
Would it help if we offered hooks that can interop with swift-lifecycle style callbacks nicely?

// pseudo code
let migrator = DatabaseMigrator()
lifecycle.register(
    label: "tracing",
    start: .sync { bootstrap tracer},
    shutdown: .async(shutdown tracer)
)

This is again one of those examples which highlight the need for swift-server/swift-service-lifecycle#11 because we could express it as:

lifecycle.register(
    label: "tracing",
    start: .sync { 
         let tracer = MyTracer()
         Instrumentation.bootstrap(...)
         return tracer
    },
    shutdown: .async { tracer in 
         tracer.shutdown(callback: $)) 
    }
)

So that'd be nice.

OR, do we need a shutdown(callback) on all instruments? We've so far avoided that because neither does swift-metrics deal with this and it just says whoever started things needs to close them, so we've taken the same road.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ktoso

OR, do we need a shutdown(callback) on all instruments?

this is related but could be handled outside of TracingInstrument API IMO

my problem is more with flushing:

  • in my case (but I expect this is going to be common) I emit segments (spans) data using UDP client which uses SwiftNIO EventLoop
  • I would like to share the EventLoop if possible, this is already implemented and works
  • in case of lambda I need to flush after each invvocation
  • XRayRecoder provides a method to flush on provided loop which allows to "async flush" but them hop in within invocation
  • now, the problem is, TracingInstrument only defined sync forceFlush - which I provided as ~flush(eventLoop).wait(); this will work but not if EventLoop is shared

TL;DR I want to change API to TracingInstrument as soon as its release.
I cannot use its forceFlush if XRaySDK shares event loop with lambda runtime (which it should...)

Copy link
Contributor

@ktoso ktoso Aug 21, 2020

Choose a reason for hiding this comment

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

I'd argue forceFlush() is a signal not a blocking function. don't wait on it, do your best, and your shutdown can "wait" until flushes are complete (because it can have either a callback version, or just be fully blocking whichever works). Wouldn't this solve the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue forceFlush() is a signal not a blocking function. don't wait on it, do your best,

I could implement forceFlush() that way.

This would mean, however, that flashing could be finished after lambda returns results (because TracingInstrument would not provide API to guarantee that instrument was flushed).

I do not know how gentle AWS is when scaling down lambda instances and if we can assume that shutdown would be called at all. (@fabianfett do you know about it?)

This would probably work most of the time for lambda + xray as flushing of XRay is comparatively cheap: UDP, local network, no DNS;
Flushing of another tracer is going to be much more expensive (and it still should work, even if not practical, right?).

private let eventLoop: EventLoop
private let allocator: ByteBufferAllocator

private var isGettingNextInvocation = false

init(eventLoop: EventLoop, configuration: Configuration) {
init(eventLoop: EventLoop, configuration: Configuration, tracer: TracingInstrument = NoOpTracingInstrument()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since this internal I prefer no default and just pipe the right things explicitly

@@ -58,7 +62,7 @@ extension Lambda {
}

/// Reports a result to the Runtime Engine.
func reportResults(logger: Logger, invocation: Invocation, result: Result<ByteBuffer?, Error>) -> EventLoopFuture<Void> {
func reportResults(logger: Logger, invocation: Invocation, result: Result<ByteBuffer?, Error>, context: BaggageContext) -> EventLoopFuture<Void> {
Copy link
Contributor

@tomerd tomerd Aug 17, 2020

Choose a reason for hiding this comment

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

in some places we refer to BaggageContext as context argument and in other places its referred to as baggage. I assume we all agree we need to be consistent. Maybe if not too late (and if makes sense) we should just rename the type to the short form Baggage and refer to it baggage since in most frameworks there will be some kind of Context that is the container/carrier.

@ktoso WDYT?

Copy link
Contributor

@tomerd tomerd Aug 17, 2020

Choose a reason for hiding this comment

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

ah saw more discussion to this end above. looks like this is still a bit of area of confusion so maybe good feedback for the API and naming choices. Lets see if we can come with something that is intuitive otherwise we will end up having this discussion every time

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah very much agreed... We wrote thought this through and there are documented guidelines here: https://github.com/slashmo/gsoc-swift-baggage-context#argument-namingpositioning

Specifically, it (and any other context) basically always should be context; a parameter should never be called baggage; the ONLY place there can be baggage is in a BaggageContextCarrier (which are always a "framework context") in order to avoid context.context it becomes context.baggage but since libraries should accept carriers rather than baggage itself most of the time you can always pass it like this let context: Lambda.Context (is ...Carrier) into get(..., context: context) since http client would accept ...Carrier (the protocol, rather than the specific context type).

// I'm very open to finding better names. Will try to brainstorm this some more with feedback from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using just the name Baggage we should think some more about if that's an option... That's going to be a standard name: https://w3c.github.io/baggage/ and it matches pretty much what we are TBH, a bag of pretty structured data.

BUT there is also https://www.w3.org/TR/trace-context/ and their relationship still remains messy (in the standards to be honest).

BaggageContext is a term known from PivotTracing and TracingPlane and somewhat "known" in the tracing space...

Open question really, but we need to be careful here :~

Copy link
Contributor

Choose a reason for hiding this comment

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

personally, naming it just Baggage and the a framework context is a BaggageCarrier (cute) or BaggageContainer (technical) seems compelling but I may be missing use cases where this does not work?

private var _baggage: BaggageContext

/// Baggage context.
public var baggage: BaggageContext {
Copy link
Contributor

@tomerd tomerd Aug 17, 2020

Choose a reason for hiding this comment

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

does this need to be a public var vs let? is it immutable or would the user ever write on this field?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's currently forced to this via the Carrier's requirement, but it may well be that requirement is quite wrong and should just be get rather than get/set tbh. We'll look into that. It loops into a few pieces fo feedback @pokryfka had here 👀

@tomerd
Copy link
Contributor

tomerd commented Oct 5, 2020

hi @pokryfka if you want to keep this around, could you please update this PR to be against the main branch so we can delete the old master branch

@tomerd tomerd closed this Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion for things that need discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants