-
Notifications
You must be signed in to change notification settings - Fork 358
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
feat: inject user agent in Lambda runtime API calls #363
Conversation
lambda-runtime/src/requests.rs
Outdated
@@ -20,6 +20,7 @@ impl IntoRequest for NextEventRequest { | |||
fn into_req(self) -> Result<Request<Body>, Error> { | |||
let req = Request::builder() | |||
.method(Method::GET) | |||
.header("User-Agent", format!("aws-lambda-rust/{}", env!("CARGO_PKG_VERSION"))) |
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.
format!()
is 3 times slower than [].concat()
. See https://github.com/hoodie/concatenation_benchmarks-rs. I've just re-run the benches on the latest nightly and it gave me 23ns vs 79ns on my laptop.
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.
Thanks! I've changed it to ["aws-lambda-rust/", env!("CARGO_PKG_VERSION")].concat()
.
Question from me: since both of these values are known at compile time, wouldn't the compiler already optimize them to prevent concatenation at run time?
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.
Hey! I actually ran into this recently in another project. I think it comes down to concat
being able to call through to an implementation that is specifically optimized for combining the two &str
inputs. format!
doesn't have this due to the need for being extremely flexible.
@nmoutschen anything those of us using custom Rust runtimes (not this crate) can do to signal our usage and support of Rust to AWS? |
@SebastianEdwards If you use a similar pattern (/) for the User-Agent, and you let me know what pattern you'll use, we can track it. 😁 I'll just ask that you don't prefix your runtime name with "aws-lambda". E.g. if you made a runtime called "sebs-awesome-runtime", you can pass a "sebs-awesome-runtime/0.1.0" User-Agent to the Lambda runtime API. |
Haha, alright, I guess the user-agent is just going to have to be "sebs-awesome-runtime" then. Will add the header next week |
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.
You can avoid allocations for User-Agent
by defining it once as a const:
const USER_AGENT: &'static str = concat!("aws-lambda-rust/", env!("CARGO_PKG_VERSION"));
See https://godbolt.org/z/8fPdxWKrn for a comparison on the generated ASM.
Change overall looks good to me. I agree with @simonvandel that we could probably get away with making this a const using the above. This way we avoid all of the additional allocations on request and response. Thoughts @nmoutschen? |
Sounds good! I'll make the changes tomorrow morning. |
b2e32f9
to
2e6404d
Compare
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.
This looks good. Thanks for addressing the feedback!
Issue #, if available:
Description of changes:
This injects a User-Agent header into the Lambda runtime API calls to track usage of Rust for provided/provided.al2 Lambda runtimes.
This uses
aws-lambda-rust/%s
, with %s being the crate version, to mimick the behaviour of other runtimes. E.g. for Go, Java RIC. This is slightly different as we cannot get the rustc version directly from the environment variables set by Cargo.By submitting this pull request