feat(node): Add Sentry tRPC middleware #7511

lforst · 2023-03-19T02:49:54Z

This PR adds tRPC support.

Why? tRPC usually requires you to define a particular route of your http server to handle RPC requests. If not instrumented, these requests will show up in the performance product under one transaction name.

The Sentry tRPC middleware will take care of meaningfully naming the transactions and attaches some additional information if configured.

lforst · 2023-03-19T02:53:48Z

cc @kamilogorek :P

packages/node/src/trpc.ts

kamilogorek · 2023-03-19T09:28:06Z

packages/node/src/index.ts

@@ -65,6 +65,8 @@ const INTEGRATIONS = {

 export { INTEGRATIONS as Integrations, Handlers };

+export { sentryTrpcMiddleware } from './trpc';


Not sure if lib-specific code should be exported directly from our core node package tbh. And if so, we can maybe put it under handlers.ts, where our express-related code lives already?

I wouldn't know where else to put it. I think it is fine here, considering we also have stuff like specific DB integrations exported from the node pkg. Do you know a better place?

Also, I wouldn't put it into handlers. It's not really a handler lol. Also, I don't think it matters too much in which file we put it. I just wanted to be able to find it quickly if I fuzzy search for "trpc".

I think it is fine here, considering we also have stuff like specific DB integrations exported from the node pkg.

We don't, it's exported from tracing package :P

It's not really a handler lol.

Neither are express request and tracing "middlewares" that we call handlers, yet we expose them via Handlers :P

wdyt @AbhiPrasad? If we leave it here, I'd simplify the name to tRPCMiddleware as import 'sentrySomething' from 'sentry' sounds strange.

We don't, it's exported from tracing package :P

This will change, we want to rm -rf the tracing package. #7346

I'd simplify the name to tRPCMiddleware as import 'sentrySomething' from 'sentry' sounds strange.

Sounds fine to me. Only concern we might have is that it might collide with other packages, which might be annoying for vscode autocomplete (not a huge deal, but still some DX we need to think about).

kamilogorek · 2023-03-19T09:35:32Z

packages/node/src/trpc.ts

+        procedureType: type,
+      };
+
+      if (options.attachRpcInput !== undefined ? options.attachRpcInput : clientOptions?.sendDefaultPii) {


This might be somewhat confusing to users. sendDefaultPii is documented as something that controls headers and cookies, not the user-provided input.
I'd probably change it to options.attachRpcInput and add default to options: SentryTrpcMiddlewareOptions = {}, which is not currently set as optional anyway.

sendDefaultPii has become much broader lately. We have a few places where we send additional data if sendDefaultPii is set to true, asides from just cookies and header. The docs just state cookies and headers as an example.

You're right about the optional options object. Thanks! --> 4d16e63

kamilogorek · 2023-03-19T09:37:16Z

packages/node/src/trpc.ts

+    const sentryTransaction = hub.getScope()?.getTransaction();
+
+    if (sentryTransaction) {
+      sentryTransaction.setName(`${path}()`, 'route');


Is this standardized format for RPC? We use urls directly as route value in other places, sometimes prepended with a METHOD.

I just use the path without added () when logging myself

I asked the team if we have precedence or a convention around transaction names for RPCs. I believe it comes down to preference of how you would like it to look in the product.

Here, for tRPC at least, imo it doesn't make too much sense to show the METHOD in the transaction name since it is always POST anyways (correct me if I am wrong ^^) and it would just be noise. The method name is still included in the span details if the server is properly instrumented otherwise.

Considering the point about noise above, I think we should drop the (). It just makes the transaction name more lengthy without any real value. I believe I added it at first because it denoted quite well that something was a method call.

We should match OpenTelemetry here, which names their spans as: $package.$service/$method.

We do this already in our OpenTelemetry transformations across the different SDKs.

packages/node/src/trpc.ts

kamilogorek · 2023-03-19T09:43:02Z

packages/node/src/trpc.ts

+
+    if (sentryTransaction) {
+      sentryTransaction.setName(`${path}()`, 'route');
+      sentryTransaction.op = 'rpc.server';


https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/rpc.md I don't see rpc.server operation in the spec we follow, there's rpc only. Do we make a distinction here ourselves?

In general, I think it is fine to append stuff to the span op. I think there is a hierarchy to it. The docs are just a guide to set the baseline ops we should use.

Here I decided to append .server because we could at some point decide that we would like to instrument the client side part of RPCs and there we would probably call it rpc.client. Similar to how we do it with http.server and http.client.

Yeah this is fine, we can update our span operations spec to account for this https://develop.sentry.dev/sdk/performance/span-operations/

Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>

KATT · 2023-03-20T10:52:37Z

packages/node/src/trpc.ts

+      sentryTransaction.setData('trpc', trpcData);
+    }
+
+    return next();


You might want to log something if the procedure actually fails? https://trpc.io/docs/server/middlewares#logging

Good point, but for now I don't think we need to add this. People will still need to use this integration with a proper server framework integration like the Express integration for it to work, and those integrations will handle the errors.

We could at some point add an option that allows capturing of non-200 responses.

unblock

github-actions · 2023-03-29T14:32:58Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.62 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	64.4 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.15 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	56.78 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	21.53 KB (0%)
@sentry/browser - Webpack (minified)	72 KB (0%)
@sentry/react - Webpack (gzipped + minified)	21.55 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	52 KB (+0.05% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.19 KB (+0.1% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.39 KB (+0.1% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	44.74 KB (0%)
@sentry/replay - Webpack (gzipped + minified)	38.86 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	63.43 KB (+0.04% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	56.49 KB (0%)

AbhiPrasad · 2023-03-29T14:33:42Z

packages/node/src/index.ts

@@ -65,6 +65,8 @@ const INTEGRATIONS = {

 export { INTEGRATIONS as Integrations, Handlers };

+export { trpcMiddleware } from './trpc';


h: let's put this under Handlers for now. I don't think we need to worry about tree-shaking and this matches the pattern we have with the rest of the SDK. We can re-evaluate this at a later time.

Done! f74e161

AbhiPrasad · 2023-03-30T07:36:08Z

packages/node/src/handlers.ts

+        trpcData.procedureInput = normalize(rawInput);
+      }
+
+      sentryTransaction.setData('trpc', trpcData);


transaction.setData does nothing on the event atm - can we add it as context instead? If we do that, can just just add it globally on the scope so that errors also get this info?

I think we had this exact discussion before ^^ Data definitely appears in the transaction view:

But I agree that a context is more valuable: 6ff778e

Ah it does appear in trace context!!

Keep forgetting 🤦

AbhiPrasad · 2023-03-30T07:36:13Z

packages/node/src/handlers.ts

@@ -315,6 +316,48 @@ export function errorHandler(options?: {
  };
 }

+interface SentryTrpcMiddlewareOptions {
+  attachRpcInput?: boolean;


m: can we add a doc string?

👍 8ab4c72

AbhiPrasad

l: IMO we should use snake_vas vars since that matches the convention of every single other context we set. If we want to make it look nice in the UI we can make frontend changes there.

But I'm fine to merge without this.

AbhiPrasad · 2023-03-30T08:03:44Z

packages/node/src/handlers.ts

      };

      if (options.attachRpcInput !== undefined ? options.attachRpcInput : clientOptions?.sendDefaultPii) {
-        trpcData.procedureInput = normalize(rawInput);
+        trpcContext['Input'] = normalize(rawInput);


Suggested change

trpcContext['Input'] = normalize(rawInput);

trpcContext['input'] = normalize(rawInput);

AbhiPrasad · 2023-03-30T08:03:53Z

packages/node/src/handlers.ts

-      const trpcData: Record<string, unknown> = {
-        procedureType: type,
+      const trpcContext: Record<string, unknown> = {
+        'Procedure Type': type,


Suggested change

'Procedure Type': type,

procedure_type: type,

lforst · 2023-03-30T08:13:48Z

l: IMO we should use snake_vas vars since that matches the convention of every single other context we set. If we want to make it look nice in the UI we can make frontend changes there.

Couldn't find any cases in the JS repo where we set context with snake case except for in tests but I am fine with it.

hichemfantar · 2023-07-11T00:03:49Z

Is there an example repo on how to use this in the t3 stack?
trpc/trpc#1934

AbhiPrasad · 2023-07-11T14:23:26Z

Hey @hichemfantar - I wrote up a small guide for using Sentry with the t3 stack here: #8500

hichemfantar · 2023-07-11T23:57:54Z

Hello, @AbhiPrasad - I'm trying to have the setup in t3 turbo but it's not working for some reason.
With t3 turbo, the trpc router lives in a separate package.
I tried different approaches but it's not working.
Trying to detect errors on the api side and reporting them is not working.
`apps\business-dashboard\src\pages\api\trpc[trpc].ts

import * as Sentry from "@sentry/nextjs";

// export API handler
export default withCors(
  createNextApiHandler({
    router: businessApiRouter,
    createContext: createTRPCContext,
    /**
 * @link https://trpc.io/docs/error-handling
 */
    onError({ error }) {
      Sentry.captureException(error);
      Sentry.captureMessage("Something went wrong");

      if (error.code === 'INTERNAL_SERVER_ERROR') {
        // send to bug reporting

        console.error('Something went wrong', error);
      }
    },
    // for some reason the 'any' is needed here
  }) as any);

```

I also tried setting a middleware in this file `https://github.com/t3-oss/create-t3-turbo/blob/main/packages/api/src/trpc.ts` but it's not working even tho it's returning 500.

```ts
import * as Sentry from "@sentry/node";

Sentry.init({
  dsn: "https://4778f57e03c4439ca06b662a083a2d02@o4505510253887488.ingest.sentry.io/4505510279970816",
  integrations: [new CaptureConsole()],
  // We recommend adjusting this value in production, or using tracesSampler
  // for finer control
  tracesSampleRate: 1.0,
});

const sentryMiddleware = t.middleware(
  Sentry.Handlers.trpcMiddleware({
    attachRpcInput: true,
  })
);

export const publicProcedure = t.procedure.use(sentryMiddleware )

```

AbhiPrasad · 2023-07-12T12:15:30Z

Hey @hichemfantar, adding the debug: true flag to your Sentry.init calls makes Sentry generate debug logs, which can help tell what is going wrong.

Let's move convo into #8500 please, I'll be locking this issue.

feat(node): Add Sentry tRPC middleware

043013f

lforst marked this pull request as ready for review March 19, 2023 02:53

kamilogorek previously requested changes Mar 19, 2023

View reviewed changes

lforst and others added 3 commits March 20, 2023 03:08

make options optional

4d16e63

Don't await

7cc473d

Update packages/node/src/trpc.ts

003095f

Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>

KATT reviewed Mar 20, 2023

View reviewed changes

AbhiPrasad mentioned this pull request Mar 20, 2023

feat(node): Export tracing from @sentry/node #7503

Merged

Address review feedback

b5022f8

lforst requested a review from AbhiPrasad March 29, 2023 14:20

AbhiPrasad reviewed Mar 29, 2023

View reviewed changes

Export as part of handlers

f74e161

AbhiPrasad reviewed Mar 30, 2023

View reviewed changes

lforst added 2 commits March 30, 2023 08:01

Add doc

8ab4c72

Context instead of data

6ff778e

AbhiPrasad approved these changes Mar 30, 2023

View reviewed changes

snake case context

3586c36

lforst merged commit bb11a2e into develop Mar 30, 2023

lforst deleted the lforst-trcp-integration branch March 30, 2023 09:09

getsentry locked as resolved and limited conversation to collaborators Jul 12, 2023

		@@ -65,6 +65,8 @@ const INTEGRATIONS = {

		export { INTEGRATIONS as Integrations, Handlers };

		export { sentryTrpcMiddleware } from './trpc';

		@@ -65,6 +65,8 @@ const INTEGRATIONS = {

		export { INTEGRATIONS as Integrations, Handlers };

		export { trpcMiddleware } from './trpc';

	trpcContext['Input'] = normalize(rawInput);
	trpcContext['input'] = normalize(rawInput);

Uh oh!

feat(node): Add Sentry tRPC middleware #7511

feat(node): Add Sentry tRPC middleware #7511

Uh oh!

Conversation

lforst commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lforst commented Mar 19, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lforst Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lforst commented Mar 30, 2023

Uh oh!

hichemfantar commented Jul 11, 2023

Uh oh!

AbhiPrasad commented Jul 11, 2023

Uh oh!

hichemfantar commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhiPrasad commented Jul 12, 2023

Uh oh!

lforst commented Mar 19, 2023 •

edited

Loading

lforst Mar 20, 2023 •

edited

Loading

github-actions bot commented Mar 29, 2023 •

edited

Loading

hichemfantar commented Jul 11, 2023 •

edited

Loading