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

Add default value for traceID header #246

Merged
merged 4 commits into from
Feb 19, 2022
Merged

Conversation

StefanNienhuis
Copy link
Contributor

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.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

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

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

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

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

@tomerd
Copy link
Contributor

tomerd commented Feb 11, 2022

@swift-server-bot test this please

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.

Also we should a test to ensure, this continues to work.

@tomerd
Copy link
Contributor

tomerd commented Feb 18, 2022

thanks @StefanNienhuis see feedback inline

Co-authored-by: tomer doron <tomer@apple.com>
@StefanNienhuis
Copy link
Contributor Author

@tomerd Sorry for the delay. Should be ready now.

@fabianfett
Copy link
Member

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented Feb 18, 2022

thanks @StefanNienhuis looks like there are some formatting issues the validation script found. you can also run it locally with docker-compose -f docker/docker-compose.yaml run --rm soundness

"preLaunchTask": "swift: Build All"
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

.gitignore ?

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

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

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

Copy link
Contributor

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 374dc71

@fabianfett
Copy link
Member

@swift-server-bot test this please

@tomerd tomerd merged commit c1f694f into swift-server:main Feb 19, 2022
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