Skip to content

feat(bottlecap): add invocation span#394

Merged
duncanista merged 28 commits intojordan.gonzalez/bottlecap/universal-instrumentationfrom
jordan.gonzalez/bottlecap/create-lambda-invocation-span
Sep 27, 2024
Merged

feat(bottlecap): add invocation span#394
duncanista merged 28 commits intojordan.gonzalez/bottlecap/universal-instrumentationfrom
jordan.gonzalez/bottlecap/create-lambda-invocation-span

Conversation

@duncanista
Copy link
Copy Markdown
Contributor

@duncanista duncanista commented Sep 13, 2024

What?

Adds the aws.lambda invocation span to Universally Instrumented runtimes like .NET, Go, Java, and Ruby.

image

Motivation

#374 and SVLS-5438

Testing

  • Added some unit tests, integration tests will be added later
  • Tested manually for .NET

Notes

Some tags will be missing from the top-level span

Comment thread bottlecap/src/metrics/enhanced/constants.rs Outdated
}

#[allow(clippy::module_name_repetitions)]
pub struct ContextBuffer {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided to use the ContextBuffer so we can keep relevant data from the past 5 invocations in cache.

5 is just an arbitrary number, we can reduce/increase this number as desired.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we remove it once it's read and used? In theory this setup would allow us to drop invocation contexts which we haven't used for some reason yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When a PlatformRuntime done hits, we effectively remove the Context in question, is this what you mean?

What do you mean by drop invocation context which we haven't used for some reason yet?

Comment thread bottlecap/src/bin/bottlecap/main.rs Outdated
Comment on lines +91 to +101
let span_size = std::mem::size_of_val(&self.span);
let header_tags = tracer_header_tags::TracerHeaderTags {
lang: "",
lang_version: "",
lang_interpreter: "",
lang_vendor: "",
tracer_version: "",
container_id: "",
client_computed_top_level: false,
client_computed_stats: false,
};
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

definitely missing a lot of data here

Copy link
Copy Markdown
Contributor Author

@duncanista duncanista Sep 20, 2024

Choose a reason for hiding this comment

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

Added a todo comment, not sure how we can get these ones easily

Comment on lines +86 to +88
span.meta
.insert("request_id".to_string(), request_id.clone());
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Missing a bunch of other tags here too

Comment thread bottlecap/src/bin/bottlecap/main.rs
@duncanista duncanista marked this pull request as ready for review September 20, 2024 17:45
@duncanista duncanista requested a review from a team as a code owner September 20, 2024 17:45
Comment on lines +418 to +420
lambda_enhanced_metrics
.set_runtime_duration_metric(metrics.duration_ms);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I might want to delegate some of this metrics to the lifecycle processor eventually, maybe?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it'd be good to push this out somewhere which can do the work asynchronously, you're blocking the loop right now

Event::Telemetry(event) =>
match event.record {
TelemetryRecord::PlatformStart { request_id, .. } => {
let mut p = invocation_processor.lock().await;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need a lock here? this prevents us from processing any messages until it's acquired, what's it fully protecting here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking what improvements I can make here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As per our conversation, will move the lock out of this in another PR when the time comes, will add a ticket for this

@duncanista duncanista merged commit d4e37d2 into jordan.gonzalez/bottlecap/universal-instrumentation Sep 27, 2024
@duncanista duncanista deleted the jordan.gonzalez/bottlecap/create-lambda-invocation-span branch September 27, 2024 20:39
alexgallotta pushed a commit that referenced this pull request Oct 10, 2024
* decouple `hyper` from `trace_processor`

* add `handle_traces`

* fix tests

* removed unused import

* move `invocation_context` to `invocation::context` module

also added some more fields and refactored it

* add `new` and `get_sender_copy` to `trace_agent`

* add `get_canonical_resource_name` to `tags_provider`

* add `get_function_name` to `lambda::tags`

* add `MS_TO_NS` constant

* add `invocation::processor`

* update use of `invocation::context`

* make `lifecycle::listener` to use `invocation::processor`

* use `invocation::processor` in `main.rs`

* move `MS_TO_NS` to `invocation::processor`

* remove unnecessary constant

* add `Box::new` back to `trace_agent`

* add some comments

* add unit tests for `context.rs`

* use `on_invocation_start`

* rename `lambda_library_detected` to `tracer_detected`

* fmt

* remove `current_request_id`

I think we dont need it

* add comment

* fmt
alexgallotta pushed a commit that referenced this pull request Nov 4, 2024
* decouple `hyper` from `trace_processor`

* add `handle_traces`

* fix tests

* removed unused import

* move `invocation_context` to `invocation::context` module

also added some more fields and refactored it

* add `new` and `get_sender_copy` to `trace_agent`

* add `get_canonical_resource_name` to `tags_provider`

* add `get_function_name` to `lambda::tags`

* add `MS_TO_NS` constant

* add `invocation::processor`

* update use of `invocation::context`

* make `lifecycle::listener` to use `invocation::processor`

* use `invocation::processor` in `main.rs`

* move `MS_TO_NS` to `invocation::processor`

* remove unnecessary constant

* add `Box::new` back to `trace_agent`

* add some comments

* add unit tests for `context.rs`

* use `on_invocation_start`

* rename `lambda_library_detected` to `tracer_detected`

* fmt

* remove `current_request_id`

I think we dont need it

* add comment

* fmt
duncanista added a commit that referenced this pull request Nov 15, 2024
* decouple `hyper` from `trace_processor`

* add `handle_traces`

* fix tests

* removed unused import

* move `invocation_context` to `invocation::context` module

also added some more fields and refactored it

* add `new` and `get_sender_copy` to `trace_agent`

* add `get_canonical_resource_name` to `tags_provider`

* add `get_function_name` to `lambda::tags`

* add `MS_TO_NS` constant

* add `invocation::processor`

* update use of `invocation::context`

* make `lifecycle::listener` to use `invocation::processor`

* use `invocation::processor` in `main.rs`

* move `MS_TO_NS` to `invocation::processor`

* remove unnecessary constant

* add `Box::new` back to `trace_agent`

* add some comments

* add unit tests for `context.rs`

* use `on_invocation_start`

* rename `lambda_library_detected` to `tracer_detected`

* fmt

* remove `current_request_id`

I think we dont need it

* add comment

* fmt
duncanista added a commit that referenced this pull request Nov 19, 2024
* decouple `hyper` from `trace_processor`

* add `handle_traces`

* fix tests

* removed unused import

* move `invocation_context` to `invocation::context` module

also added some more fields and refactored it

* add `new` and `get_sender_copy` to `trace_agent`

* add `get_canonical_resource_name` to `tags_provider`

* add `get_function_name` to `lambda::tags`

* add `MS_TO_NS` constant

* add `invocation::processor`

* update use of `invocation::context`

* make `lifecycle::listener` to use `invocation::processor`

* use `invocation::processor` in `main.rs`

* move `MS_TO_NS` to `invocation::processor`

* remove unnecessary constant

* add `Box::new` back to `trace_agent`

* add some comments

* add unit tests for `context.rs`

* use `on_invocation_start`

* rename `lambda_library_detected` to `tracer_detected`

* fmt

* remove `current_request_id`

I think we dont need it

* add comment

* fmt
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.

2 participants