Skip to content

feat(bottlecap): create Inferred Spans baseline + infer API Gateway HTTP spans#405

Merged
astuyve merged 19 commits intojordan.gonzalez/bottlecap/universal-instrumentationfrom
jordan.gonzalez/bottlecap/add-inferred-spans
Oct 9, 2024
Merged

feat(bottlecap): create Inferred Spans baseline + infer API Gateway HTTP spans#405
astuyve merged 19 commits intojordan.gonzalez/bottlecap/universal-instrumentationfrom
jordan.gonzalez/bottlecap/add-inferred-spans

Conversation

@duncanista
Copy link
Copy Markdown
Contributor

What?

Motivation

Notes

}

#[allow(clippy::module_name_repetitions)]
#[derive(Debug, Clone)]
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 to add clone trait here?
I thought the configs were immutable and passed as ref/Arc everywhere


pub trait Trigger: Sized {
fn new(payload: Value) -> Option<Self>;
fn is_match(payload: &Value) -> bool;
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.

Would it be more convenient to have a general matching function returning a type instead of is_match for every subclass? To avoid doing is_match till something matches and maybe hiding bugs when a match hides running other match logic

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 thought so, but I wanted to not have to separate payload-specific logic into its own trait, I'm still exploring the idea, but I think this might change depending on how it would look like when we add more cases.

How would you change this code? My main constraints are: not deserialize on every match, just deserialize once into serde_json::Value and then try to find the actual type of the trigger, wdyt?


fn generate_span_id() -> u64 {
// todo: secure random id with OsRng for SnapStart
let mut rng = rand::thread_rng();
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.

we should do this before merge, I can take a look if you want

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.

Ok, should be a simple fix, will look into it tomorrow – what do you mean by tags like cold start?

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.

You've got a comment in the code about adding tags for cold starts/proactive inits on the span

Copy link
Copy Markdown
Contributor

@astuyve astuyve left a comment

Choose a reason for hiding this comment

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

looks good to me except rng and tags like cold start.

@astuyve astuyve marked this pull request as ready for review October 9, 2024 18:56
@astuyve astuyve requested a review from a team as a code owner October 9, 2024 18:56
Base automatically changed from jordan.gonzalez/bottlecap/add-trace-propagator to jordan.gonzalez/bottlecap/universal-instrumentation October 9, 2024 18:59
@astuyve astuyve merged commit b61fe08 into jordan.gonzalez/bottlecap/universal-instrumentation Oct 9, 2024
@astuyve astuyve deleted the jordan.gonzalez/bottlecap/add-inferred-spans branch October 9, 2024 18:59
alexgallotta pushed a commit that referenced this pull request Nov 4, 2024
…TTP spans (#405)

* add `Trigger` trait for inferred spans

* add `ApiGatewayHttpEvent` trigger

* add `SpanInferrer`

* make `invocation::processor` to use `SpanInferrer`

* send `aws_config` to `invocation::processor`

* use incoming payload for `invocation::processor` for span inferring

* add `api_gateway_http_event.json` for testing

* add `api_gateway_proxy_event.json` for testing

* fix: Convert tag hashmap to sorted vector of tags

* fix: fmt

---------

Co-authored-by: AJ Stuyvenberg <astuyve@gmail.com>
duncanista added a commit that referenced this pull request Nov 15, 2024
…TTP spans (#405)

* add `Trigger` trait for inferred spans

* add `ApiGatewayHttpEvent` trigger

* add `SpanInferrer`

* make `invocation::processor` to use `SpanInferrer`

* send `aws_config` to `invocation::processor`

* use incoming payload for `invocation::processor` for span inferring

* add `api_gateway_http_event.json` for testing

* add `api_gateway_proxy_event.json` for testing

* fix: Convert tag hashmap to sorted vector of tags

* fix: fmt

---------

Co-authored-by: AJ Stuyvenberg <astuyve@gmail.com>
duncanista added a commit that referenced this pull request Nov 19, 2024
…TTP spans (#405)

* add `Trigger` trait for inferred spans

* add `ApiGatewayHttpEvent` trigger

* add `SpanInferrer`

* make `invocation::processor` to use `SpanInferrer`

* send `aws_config` to `invocation::processor`

* use incoming payload for `invocation::processor` for span inferring

* add `api_gateway_http_event.json` for testing

* add `api_gateway_proxy_event.json` for testing

* fix: Convert tag hashmap to sorted vector of tags

* fix: fmt

---------

Co-authored-by: AJ Stuyvenberg <astuyve@gmail.com>
duncanpharvey pushed a commit that referenced this pull request Mar 10, 2026
…TTP spans (#405)

* add `Trigger` trait for inferred spans

* add `ApiGatewayHttpEvent` trigger

* add `SpanInferrer`

* make `invocation::processor` to use `SpanInferrer`

* send `aws_config` to `invocation::processor`

* use incoming payload for `invocation::processor` for span inferring

* add `api_gateway_http_event.json` for testing

* add `api_gateway_proxy_event.json` for testing

* fix: Convert tag hashmap to sorted vector of tags

* fix: fmt

---------

Co-authored-by: AJ Stuyvenberg <astuyve@gmail.com>
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.

3 participants