swift-server / swift-aws-lambda-runtime

Swift implementation of AWS Lambda Runtime
https://swiftpackageindex.com/swift-server/swift-aws-lambda-runtime
Apache License 2.0
1.14k stars 104 forks source link

Generate trace ID in correct format (LocalLambda Server) #139

Closed pokryfka closed 4 years ago

pokryfka commented 4 years ago

Generate (X-Ray) Trace ID in correct format.

Motivation:

Generate (X-Ray) Trace ID in correct format. Helpful when testing X-Ray tracing.

References

Modifications:

Result:

Example value of Lambda-Runtime-Trace-Id header:

Root=1-5efd7f0b-da552a1eb226e4718fff8790;Sampled=1

before (root and parent values used to be signed integers):

"Root=25457;Parent=14820;Sampled=1"
swift-server-bot commented 4 years ago

Can one of the admins verify this patch?

swift-server-bot commented 4 years ago

Can one of the admins verify this patch?

swift-server-bot commented 4 years ago

Can one of the admins verify this patch?

swift-server-bot commented 4 years ago

Can one of the admins verify this patch?

pokryfka commented 4 years ago

@pokryfka This is awesome! I'm fine with this change but let's wait for @tomerd's opinion. Further, would you mind creating a test that verifies the format?

Sure, I can create a test.

In which case the function will need to be internal (its defined within private LocalLambda enum at the moment), in which case.. should I move it to Utils or keep it in Lambda+LocalServer? The function is generic though probably will only be used by the LocalLambda:Server.

@fabianfett

tomerd commented 4 years ago

@swift-server-bot test this please

tomerd commented 4 years ago

thanks @pokryfka this is great. I think there are also places in the unit tests where we fake the traceID and would be nice to use a more correct one.

wondering if we should rename the function to be generateXRayTraceID to clarify the intent. I also always like to add the "internal" modifier to make it super clear

pokryfka commented 4 years ago

@tomerd Thank you for the feedback. Please see 2 questions below.

I could rename the function or/and create XRay namespace?

internal enum XRay {
    static func generateTraceID() -> String {
        // ...
    }
}

XRay related functions or types would end up there if needed or practical, for example we could also have:

internal enum XRay {
    static func generateTracingHeader(parent: String? = nil, sampled: Bool? = nil) -> String {
        // ...
    }
}

At the moment LocalLambda:Server is the only place where tracing header value is generated.

MockLambdaServer in AWSLambdaRuntimeCoreTests creates the tracing header using (correctly formatted) string literal; I dont think the actual values are used during tests; should they be generated?

                responseHeaders = [
                    (AmazonHeaders.requestID, requestId),
                    (AmazonHeaders.invokedFunctionARN, "arn:aws:lambda:us-east-1:123456789012:function:custom-runtime"),
                    (AmazonHeaders.traceID, "Root=1-5bef4de7-ad49b0e87f6ef6c87fc2e700;Parent=9a9197af755a6419;Sampled=1"),
                    (AmazonHeaders.deadline, String(deadline)),
                ]
tomerd commented 4 years ago

@parkera wdyt about adding it to AmazonHeaders, ie AmazonHeaders::generateXRayTraceID?

Would be nice to have AWSLambdaRuntimeCoreTests use it as well, but not required per se

pokryfka commented 4 years ago

@tomerd made both changes, please let me know if you have more comments

tomerd commented 4 years ago

@swift-server-bot test this please

tomerd commented 4 years ago

@pokryfka looks great, one question

tomerd commented 4 years ago

@swift-server-bot test this please

tomerd commented 4 years ago

thank you @pokryfka <3