-
Notifications
You must be signed in to change notification settings - Fork 9
renaming and small refactor #627
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR performs a renaming and small refactor focused on clarifying invocation span handling and header processing.
- In bottlecap/src/lifecycle/listener.rs, the invocation handling functions are renamed and refactored to extract the request headers more explicitly.
- In bottlecap/src/lifecycle/invocation/processor.rs, the field previously named "span" is renamed to "aws_lambda_span" for improved clarity.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
bottlecap/src/lifecycle/listener.rs | Refactored invocation handlers and updated header extraction logic |
bottlecap/src/lifecycle/invocation/processor.rs | Renamed the "span" field to "aws_lambda_span" and updated its usage |
Comments suppressed due to low confidence (2)
bottlecap/src/lifecycle/invocation/processor.rs:55
- [nitpick] The comment still refers to 'span' even though the field has been renamed to 'aws_lambda_span'. It is recommended to update the comment for consistency.
// Current invocation span
bottlecap/src/lifecycle/listener.rs:64
- Destructuring the request into 'parts' and 'body' changes how the request is handled compared to passing the full request. Please double-check that none of the subsequent logic relies on properties of the original Request object.
let (parts, body) = req.into_parts();
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.
Pull Request Overview
This pull request renames key identifiers to better reflect their purpose and slightly refactors the code without altering functionality. Key changes include:
- Renaming functions in the Listener to use "universal_instrumentation_start" and "universal_instrumentation_end".
- Renaming the Processor’s "span" field to "aws_lambda_span" with corresponding updates throughout the file.
- Adjusting type signatures and helper functions to accept header references instead of owned header maps.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
bottlecap/src/lifecycle/listener.rs | Renamed handler functions and updated the headers conversion helper signature. |
bottlecap/src/lifecycle/invocation/processor.rs | Renamed the span field to aws_lambda_span and updated related references. |
Comments suppressed due to low confidence (2)
bottlecap/src/lifecycle/listener.rs:101
- [nitpick] Consider updating the debug log message to reflect the new 'universal_instrumentation_start' naming (e.g., 'Received universal instrumentation start request') to maintain clarity in log outputs.
debug!("Received start invocation request");
bottlecap/src/lifecycle/invocation/processor.rs:151
- [nitpick] Update the comment to specify that the 'aws_lambda_span' context is being reset, ensuring the documentation remains in sync with the renamed variable.
// Reset Span Context on Span
@@ -55,7 +55,7 @@ pub struct Processor { | |||
// Helper to infer span information | |||
inferrer: SpanInferrer, | |||
// Current invocation span | |||
pub span: Span, | |||
pub aws_lambda_span: Span, |
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.
What about invocation_span
? Since this is the invocation/processor
?
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.
aws.lambda.span is very specific, and I think it fits this case the best. There is no other span that would end in that place, right?
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.
Pull Request Overview
This PR performs a non‐functional refactor focused on renaming to improve clarity in the instrumentation code. Key changes include:
- Renaming listener function handlers and their associated constants to "UNIVERSAL_INSTRUMENTATION_*".
- Updating the headers mapping function to take a reference for improved clarity.
- Renaming the Processor's "span" field to "aws_lambda_span" along with all related references for consistent semantics.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
bottlecap/src/lifecycle/listener.rs | Renamed constants and function handlers, updated header mapping signature. |
bottlecap/src/lifecycle/invocation/processor.rs | Renamed the span field to aws_lambda_span and updated all references accordingly. |
Comments suppressed due to low confidence (2)
bottlecap/src/lifecycle/listener.rs:23
- [nitpick] The constant name 'UNIVERSAL_INSTRUMENTATION_START_INVOCATION_PATH' is quite long; consider whether a shorter yet descriptive name could improve readability.
const UNIVERSAL_INSTRUMENTATION_START_INVOCATION_PATH: &str = "/lambda/start-invocation";
bottlecap/src/lifecycle/invocation/processor.rs:352
- [nitpick] For consistency, consider either always using 'std::mem::size_of_val' or importing it explicitly so that its usage remains uniform across the file.
let mut body_size = size_of_val(&self.aws_lambda_span);
some renaming without functional changes