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(core): Add trace function #7556

Merged
merged 1 commit into from Mar 22, 2023
Merged

feat(core): Add trace function #7556

merged 1 commit into from Mar 22, 2023

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Mar 21, 2023

Sentry.trace is a wrapper that abstracts away much of the complexities that come with managing spans/transaction. You can wrap a method, and not worry about things being sync/async, all of it will be handled for you.

You can also pass in a onError callback, that can process errors (but do not actually suppress the error - users can manually do that themselves).

This was introduced to make adding Sveltekit performance monitoring more simple. As I was adding code for #7536 and #7537, I noticed that we duplicate this span creation/finishing logic all over the code base.

look at something like

fill(pkg, 'execute', function (orig: () => void | Promise<unknown>) {

or

return function (this: unknown, options: unknown, values: unknown, callback: unknown) {

It is marked as internal and private API - so we should have the freedom to break this, but I suspect the API will not change.

Ideally this means every single implementation here can just use the Sentry.trace method: https://github.com/getsentry/sentry-javascript/tree/develop/packages/tracing-internal/src/node/integrations

This breaks for client-side environments since scope propagation is busted in browser, but with domains is fine for node, so lets :shipit:

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.44 KB (+0.13% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 63.55 KB (+0.08% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.13 KB (+0.15% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 56.59 KB (+0.1% 🔺)
@sentry/browser - Webpack (gzipped + minified) 21.6 KB (+0.12% 🔺)
@sentry/browser - Webpack (minified) 71.67 KB (+0.08% 🔺)
@sentry/react - Webpack (gzipped + minified) 21.63 KB (+0.12% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 51.96 KB (+0.05% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.77 KB (+0.1% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.08 KB (+0.11% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 44.22 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.28 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 62.4 KB (+0.05% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 55.95 KB (+0.07% 🔺)

@AbhiPrasad
Copy link
Member Author

801e51e in #7536 is an example of how using the trace function can massively simplify code.

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 like this new and simplified API a lot!

*/
export function trace<T>(
context: TransactionContext,
callback: (span: Span) => T,
Copy link
Member

Choose a reason for hiding this comment

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

super-l: WDYT about renaming callback to something like traceTarget / traceSubject or something along these lines? The way I understand this function, we want to start a txn or a span for this callback so I'd say we should give it a good name. callback sounds a little generic to me.
But ofc we can (and probably will) iterate on this function in the future, so really, feel free to disregard this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to use callback for now since this is the language we are using for the cross SDK discussions about this, but you do make a good point. I'll come back and re-visit this after we chat about it as a group.


it('creates a transaction', async () => {
let ref: any = undefined;
client.on('finishTransaction', transaction => {
Copy link
Member

Choose a reason for hiding this comment

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

Making good use of hooks - I love it 😄

@AbhiPrasad AbhiPrasad merged commit 4f34b5a into develop Mar 22, 2023
52 checks passed
@AbhiPrasad AbhiPrasad deleted the abhi-trace-func branch March 22, 2023 10:40
@Mr0grog
Copy link

Mr0grog commented Mar 28, 2023

Just saw this! I’m really glad you were able to refine some of the ideas from the gist I posted back a few weeks ago in #7387 (comment).

Are there any plans to eventually make this public? I would love to be using this built-in functionality instead of hacking it in from the outside like I’ve been doing.

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

3 participants