Skip to content

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

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

Merged
merged 8 commits into from
Jul 8, 2020

Conversation

pokryfka
Copy link
Contributor

@pokryfka pokryfka commented Jul 2, 2020

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:

  • different value of Tracing header generated by LocalLambda Server with:
    • correctly formatted trace ID (root)
    • removed parent segment ID (which is optional)

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
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?

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

@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?

@pokryfka
Copy link
Contributor Author

pokryfka commented Jul 2, 2020

@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
Copy link
Contributor

tomerd commented Jul 2, 2020

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented Jul 2, 2020

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
Copy link
Contributor Author

pokryfka commented Jul 3, 2020

@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
Copy link
Contributor

tomerd commented Jul 6, 2020

@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
Copy link
Contributor Author

pokryfka commented Jul 7, 2020

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

@tomerd
Copy link
Contributor

tomerd commented Jul 7, 2020

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented Jul 7, 2020

@pokryfka looks great, one question

@tomerd tomerd added this to the 0.3.0 milestone Jul 7, 2020
@tomerd
Copy link
Contributor

tomerd commented Jul 8, 2020

@swift-server-bot test this please

@tomerd tomerd merged commit 1b594ca into swift-server:master Jul 8, 2020
@tomerd
Copy link
Contributor

tomerd commented Jul 8, 2020

thank you @pokryfka <3

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.

4 participants