-
Notifications
You must be signed in to change notification settings - Fork 113
Add default value for traceID header #246
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
Conversation
Can one of the admins verify this patch? |
11 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
@swift-server-bot test this please |
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.
Also we should a test to ensure, this continues to work.
thanks @StefanNienhuis see feedback inline |
Co-authored-by: tomer doron <tomer@apple.com>
@tomerd Sorry for the delay. Should be ready now. |
@swift-server-bot test this please |
thanks @StefanNienhuis looks like there are some formatting issues the validation script found. you can also run it locally with |
.vscode/launch.json
Outdated
"preLaunchTask": "swift: Build All" | ||
} | ||
] | ||
} |
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.
.gitignore ?
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.
I removed it from the commit for now. Do you want me to add it to .gitignore?
@@ -54,8 +54,7 @@ struct Invocation: Hashable { | |||
self.requestID = requestID | |||
self.deadlineInMillisSinceEpoch = unixTimeInMilliseconds | |||
self.invokedFunctionARN = invokedFunctionARN | |||
self.traceID = headers.first(name: AmazonHeaders.traceID) ?? | |||
"Root=\(AmazonHeaders.generateXRayTraceID());Sampled=0" | |||
self.traceID = headers.first(name: AmazonHeaders.traceID) ?? "Root=\(AmazonHeaders.generateXRayTraceID());Sampled=0" |
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.
should we add a small unit test too so this does not regress?
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.
As in a test checking the format of the generated traceID?
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.
testing that we generate a default traceID if no header is provided
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.
I have that ready. I used the header from another file and changed the year to 2022, but now soundness.sh is complaining that it's not an acceptable year. Should I add s/2022/YEARS/
to the replace_acceptable_years()
function?
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.
Should I add s/2022/YEARS/ to the replace_acceptable_years() function?
yes. sorry about the hassle!
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.
See 374dc71
@swift-server-bot test this please |
Adds a default value for the Lambda-Runtime-Trace-Id header, to make it compatible with the AWS SAM CLI and other applications depending on the Lambda Runtime Interface Emulator.
Motivation:
Currently the Swift Runtime is incompatible with the Lambda Runtime Interface Emulator.
Modifications:
Adds a default value for the Lambda-Runtime-Trace-Id header.
This is similar to what the Rust runtime is doing.
Result:
The Swift Runtime will be compatible with the Lambda Runtime Interface Emulator.