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

SDK: Trace spans black boxing #723

Merged
merged 3 commits into from
May 9, 2023
Merged

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented May 5, 2023

Internally we have a use case of not exposing sub spans when we're in context of specific trace span.

e.g. when in context of AWS SDK span we do not want to expose underlying HTTP request with extra span. So far we mitigated that with a primitive approach, where we were assuming that HTTP request is initialized in sequence synchronously and we were marking given HTTP request to not be instrumented.

This doesn't work for case where AWS SDK approaches temporary network issue and implies a retry, as then following HTTP request happens async and appears as reported, which is not wanted. Very occasionally we observed integration test failures due to that (e.g. https://github.com/serverless/console/actions/runs/4810414588/jobs/8563030944)

This PR introduces an internal concept of black box trace spans. Where we can mark span as black box and that way ensure no child spans of it will be exposed. Even if some logic will add spans to such black box span (which is not prevented in any way), they will not be exposed in final trace report, or propagated to dev mode


Additionally removed debug logs, which were added to help investigate the issue of HTTP spans being exposed when run in context of AWS SDK request

@medikoo medikoo added enhancement New feature or request runtime/node.js labels May 5, 2023
@medikoo medikoo self-assigned this May 5, 2023
@medikoo medikoo requested review from Danwakeem and Mmarzex May 5, 2023 14:50
Copy link
Contributor

@Danwakeem Danwakeem left a comment

Choose a reason for hiding this comment

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

This looks good 👍 does this mean that if you have a project built with ES build and you haven’t taken the necessary steps to expose the SDK libraries you won’t see anything?

@medikoo
Copy link
Contributor Author

medikoo commented May 8, 2023

This looks good 👍 does this mean that if you have a project built with ES build and you haven’t taken the necessary steps to expose the SDK libraries you won’t see anything?

It's not related to ES build in any way.

We instrument both HTTP requests and AWS SDK requests, yet AWS SDK request underneath issue HTTP requests, and in such case we do not want to expose spans for underlying HTTP requests. This PR improves a method in which we black box AWS SDK request spans, so HTTP request spans that happen underneath are not exposed.

@Danwakeem
Copy link
Contributor

@medikoo But if you use esbuild and your service is not configured correctly to let our SDK know that you are using esbuild the only span you will see are the underlying http requests to the AWS services.

So I am curious if we black box these http request spans does that mean that these traces (with the misconfigured sdk) will have no captured spans or will the http requests not be black boxed because we don't know they are using the AWS SDK?

Here is a sample trace where I am using esbuild and not configuring our sdk manually to know I am using the AWS SDK 👇

Screenshot 2023-05-08 at 12 19 30 PM

Would this trace not include the node.https.request span?

@medikoo
Copy link
Contributor Author

medikoo commented May 8, 2023

@Danwakeem, nothing will change in this case.

This PR is an improvement to how we black box AWS SDK spans. Note that we black box them already, but with a primitive method, which in a very rare scenario fails. This patch ensures that in this very rare scenario, it'll work as expected.

HTTP spans are black boxed only in context of AWS SDK spans. if there are no AWS SDK spans, there's no black boxing implied (this logic is initiated by AWS SDK instrumentation) So in case of ESBuild when AWS SDK instrumentation doesn't pick up, obviously HTTP spans will appear as they appear now.

@Danwakeem
Copy link
Contributor

Got it, thanks for the clarification 👍

@medikoo medikoo merged commit 12ef23b into main May 9, 2023
@medikoo medikoo deleted the 0505-improve-internal-span-filtering branch May 9, 2023 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request runtime/node.js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants