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

feat(tracing): Log start and end of spans #5446

Merged
merged 1 commit into from Jul 27, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jul 22, 2022

Right now, if one of our SDKs is running on a cloud provider and therefore all the user has is logs, there's no real way to debug missing spans. Are they not finishing in time and getting dropped? Are they even starting in the first place? No way to know.

This fixes that by adding logging to the start and end of spans, the same way we have with transactions.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.36 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 59.92 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.92 KB (-0.02% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.79 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.69 KB (0%)
@sentry/browser - Webpack (minified) 64.13 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.71 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.15 KB (+0.31% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.81 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.06 KB (-0.01% 🔽)

@lobsterkatie lobsterkatie force-pushed the kmclb-log-start-and-end-of-span branch 2 times, most recently from a93cfe3 to 0e74809 Compare July 22, 2022 02:38
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I'm just wondering: Has this problem come up before? I think it's a good idea to log more information but considering that transactions can have a lot of spans, this could mean quite a lot of logs. Plus a few bytes more in the bundle for people that don't tree-shake out debug logging.

But those are overall not real concerns...

packages/tracing/src/span.ts Outdated Show resolved Hide resolved
@lobsterkatie
Copy link
Member Author

I'm just wondering: Has this problem come up before?

It's come up for me in my vercel/nextjs testing in the past (all you have there is the logs from the serverless functions your app gets turned into), but what actually prompted this was a user yesterday who's not seeing DB spans when his app is deployed to Heroku, and me feeling like I had zero way to debug that.

I think it's a good idea to log more information but considering that transactions can have a lot of spans, this could mean quite a lot of logs.

It's funny, I almost added a sentence to the end of the PR description like, "It's true this may get a little spammy for users, but let's add it in and see how much of an issue that is vs how useful it seems in terms of debugging." So, consider it added. 🙂

Plus a few bytes more in the bundle for people that don't tree-shake out debug logging.

Yeah, this is something I considered, too. We haven't really discussed/decided how free we should now feel to add logging, given that users now have the ability to treeshake it out. I wonder if we should think about doing that automatically in the webpack plugin, and/or making a bigger deal about it/suggesting it more strongly in the docs.

WDYT (about any or all of the above)?

@Lms24
Copy link
Member

Lms24 commented Jul 22, 2022

So about spammy logs: let's see how it goes. I think it's to revert this if people complain.

About the size aspect: Thinking about it, I guess it's not a problem at this point (especially because users now can do something about it). But I think we should nevertheless be mindful of adding new log messages (also because of the spam aspect ofc).

doing that automatically in the webpack plugin

I'd vote yes on that (as long as we provide the option to continue including it ofc)

making a bigger deal about it/suggesting it more strongly in the docs

Also worth considering. We could link to the tree-shaking guide from the main install page (e.g. in the "Next Steps" section).

But I think both things can happen whenever. No immediate action required

@lobsterkatie lobsterkatie force-pushed the kmclb-log-start-and-end-of-span branch from 0e74809 to c8ae3ae Compare July 26, 2022 18:35
@lobsterkatie lobsterkatie requested a review from Lms24 July 27, 2022 01:12
@Lms24 Lms24 merged commit b673bc3 into master Jul 27, 2022
@Lms24 Lms24 deleted the kmclb-log-start-and-end-of-span branch July 27, 2022 09:55
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.

None yet

2 participants