move node-version- and server-name-adding code to client

lobsterkatie · lobsterkatie · commit 5cee1d95373a · 2022-06-14T20:17:15.000-07:00
diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts
@@ -2,6 +2,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core';
 import { SessionFlusher } from '@sentry/hub';
 import { Event, EventHint, Severity, SeverityLevel } from '@sentry/types';
 import { logger, resolvedSyncPromise } from '@sentry/utils';
+import * as os from 'os';
 import { TextEncoder } from 'util';
 
 import { eventFromMessage, eventFromUnknownInput } from './eventbuilder';
@@ -139,9 +140,15 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
    */
   protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike<Event | null> {
     event.platform = event.platform || 'node';
-    if (this.getOptions().serverName) {
-      event.server_name = this.getOptions().serverName;
-    }
+    event.contexts = {
+      ...event.contexts,
+      runtime: {
+        name: 'node',
+        version: global.process.version,
+      },
+    };
+    event.server_name =
+      event.server_name || this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname();
     return super._prepareEvent(event, hint, scope);
   }
 
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -14,7 +14,6 @@ import {
 import * as cookie from 'cookie';
 import * as domain from 'domain';
 import * as http from 'http';
-import * as os from 'os';
 import * as url from 'url';
 
 import { NodeClient } from './client';
@@ -292,10 +291,8 @@ export function extractRequestData(
 export interface ParseRequestOptions {
   ip?: boolean;
   request?: boolean | string[];
-  serverName?: boolean;
   transaction?: boolean | TransactionNamingScheme;
   user?: boolean | string[];
-  version?: boolean;
 }
 
 /**
@@ -311,23 +308,11 @@ export function parseRequest(event: Event, req: ExpressRequest, options?: ParseR
   options = {
     ip: false,
     request: true,
-    serverName: true,
     transaction: true,
     user: true,
-    version: true,
     ...options,
   };
 
-  if (options.version) {
-    event.contexts = {
-      ...event.contexts,
-      runtime: {
-        name: 'node',
-        version: global.process.version,
-      },
-    };
-  }
-
   if (options.request) {
     // if the option value is `true`, use the default set of keys by not passing anything to `extractRequestData()`
     const extractedRequestData = Array.isArray(options.request)
@@ -339,10 +324,6 @@ export function parseRequest(event: Event, req: ExpressRequest, options?: ParseR
     };
   }
 
-  if (options.serverName && !event.server_name) {
-    event.server_name = global.process.env.SENTRY_NAME || os.hostname();
-  }
-
   if (options.user) {
     const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, options.user) : {};
 
diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts
@@ -1,4 +1,6 @@
 import { Scope, SessionFlusher } from '@sentry/hub';
+import { Event, EventHint } from '@sentry/types';
+import * as os from 'os';
 
 import { NodeClient } from '../src';
 import { getDefaultNodeClientOptions } from './helper/node-client-options';
@@ -187,6 +189,81 @@ describe('NodeClient', () => {
       expect(requestSession!.status).toEqual('ok');
     });
   });
+
+  describe('_prepareEvent', () => {
+    test('adds platform to event', () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN });
+      client = new NodeClient(options);
+
+      const event: Event = {};
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.platform).toEqual('node');
+    });
+
+    test('adds runtime context to event', () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN });
+      client = new NodeClient(options);
+
+      const event: Event = {};
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.contexts?.runtime).toEqual({
+        name: 'node',
+        version: process.version,
+      });
+    });
+
+    test('adds server name to event when value passed in options', () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, serverName: 'foo' });
+      client = new NodeClient(options);
+
+      const event: Event = {};
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.server_name).toEqual('foo');
+    });
+
+    test('adds server name to event when value given in env', () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN });
+      client = new NodeClient(options);
+      process.env.SENTRY_NAME = 'foo';
+
+      const event: Event = {};
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.server_name).toEqual('foo');
+
+      delete process.env.SENTRY_NAME;
+    });
+
+    test('adds hostname as event server name when no value given', () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN });
+      client = new NodeClient(options);
+
+      const event: Event = {};
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.server_name).toEqual(os.hostname());
+    });
+
+    test("doesn't clobber existing server name", () => {
+      const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, serverName: 'bar' });
+      client = new NodeClient(options);
+
+      const event: Event = { server_name: 'foo' };
+      const hint: EventHint = {};
+      (client as any)._prepareEvent(event, hint);
+
+      expect(event.server_name).toEqual('foo');
+      expect(event.server_name).not.toEqual('bar');
+    });
+  });
 });
 
 describe('flush/close', () => {
diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts
@@ -2,7 +2,7 @@ import * as sentryCore from '@sentry/core';
 import * as sentryHub from '@sentry/hub';
 import { Hub } from '@sentry/hub';
 import { Transaction } from '@sentry/tracing';
-import { Baggage, Runtime } from '@sentry/types';
+import { Baggage } from '@sentry/types';
 import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils';
 import * as http from 'http';
 import * as net from 'net';
@@ -55,25 +55,6 @@ describe('parseRequest', () => {
     };
   });
 
-  describe('parseRequest.contexts runtime', () => {
-    test('runtime name must contain node', () => {
-      const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
-      expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node');
-    });
-
-    test('runtime version must contain current node version', () => {
-      const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
-      expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version);
-    });
-
-    test('runtime disbaled by options', () => {
-      const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
-        version: false,
-      });
-      expect(parsedRequest).not.toHaveProperty('contexts.runtime');
-    });
-  });
-
   describe('parseRequest.user properties', () => {
     const DEFAULT_USER_KEYS = ['id', 'username', 'email'];
     const CUSTOM_USER_KEYS = ['custom_property'];
diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts
@@ -180,11 +180,6 @@ function enhanceScopeWithEnvironmentData(scope: Scope, context: Context, startTi
   scope.setTag('server_name', process.env._AWS_XRAY_DAEMON_ADDRESS || process.env.SENTRY_NAME || hostname());
   scope.setTag('url', `awslambda:///${context.functionName}`);
 
-  scope.setContext('runtime', {
-    name: 'node',
-    version: global.process.version,
-  });
-
   scope.setContext('aws.lambda', {
     aws_request_id: context.awsRequestId,
     function_name: context.functionName,
diff --git a/packages/serverless/src/gcpfunction/cloud_events.ts b/packages/serverless/src/gcpfunction/cloud_events.ts
@@ -2,12 +2,7 @@ import { captureException, flush, getCurrentHub, startTransaction } from '@sentr
 import { logger } from '@sentry/utils';
 
 import { domainify, getActiveDomain, proxyFunction } from '../utils';
-import {
-  CloudEventFunction,
-  CloudEventFunctionWithCallback,
-  configureScopeWithContext,
-  WrapperOptions,
-} from './general';
+import { CloudEventFunction, CloudEventFunctionWithCallback, WrapperOptions } from './general';
 
 export type CloudEventFunctionWrapperOptions = WrapperOptions;
 
@@ -44,7 +39,7 @@ function _wrapCloudEventFunction(
     // since functions-framework creates a domain for each incoming request.
     // So adding of event processors every time should not lead to memory bloat.
     getCurrentHub().configureScope(scope => {
-      configureScopeWithContext(scope, context);
+      scope.setContext('gcp.function.context', { ...context });
       // We put the transaction on the scope so users can attach children to it
       scope.setSpan(transaction);
     });
diff --git a/packages/serverless/src/gcpfunction/events.ts b/packages/serverless/src/gcpfunction/events.ts
@@ -2,7 +2,7 @@ import { captureException, flush, getCurrentHub, startTransaction } from '@sentr
 import { logger } from '@sentry/utils';
 
 import { domainify, getActiveDomain, proxyFunction } from '../utils';
-import { configureScopeWithContext, EventFunction, EventFunctionWithCallback, WrapperOptions } from './general';
+import { EventFunction, EventFunctionWithCallback, WrapperOptions } from './general';
 
 export type EventFunctionWrapperOptions = WrapperOptions;
 
@@ -39,7 +39,7 @@ function _wrapEventFunction(
     // since functions-framework creates a domain for each incoming request.
     // So adding of event processors every time should not lead to memory bloat.
     getCurrentHub().configureScope(scope => {
-      configureScopeWithContext(scope, context);
+      scope.setContext('gcp.function.context', { ...context });
       // We put the transaction on the scope so users can attach children to it
       scope.setSpan(transaction);
     });
diff --git a/packages/serverless/src/gcpfunction/general.ts b/packages/serverless/src/gcpfunction/general.ts
@@ -1,7 +1,4 @@
-import { Scope } from '@sentry/node';
-import { Context as SentryContext } from '@sentry/types';
 import type { Request, Response } from 'express';
-import { hostname } from 'os';
 
 export interface HttpFunction {
   (req: Request, res: Response): any; // eslint-disable-line @typescript-eslint/no-explicit-any
@@ -47,19 +44,4 @@ export interface WrapperOptions {
   flushTimeout: number;
 }
 
-/**
- * Enhances the scope with additional event information.
- *
- * @param scope scope
- * @param context event context
- */
-export function configureScopeWithContext(scope: Scope, context: Context): void {
-  scope.setContext('runtime', {
-    name: 'node',
-    version: global.process.version,
-  });
-  scope.setTag('server_name', process.env.SENTRY_NAME || hostname());
-  scope.setContext('gcp.function.context', { ...context } as SentryContext);
-}
-
 export type { Request, Response };
diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts
@@ -46,8 +46,6 @@ function expectScopeSettings() {
   // @ts-ignore see "Why @ts-ignore" note
   expect(Sentry.fakeScope.setTag).toBeCalledWith('url', 'awslambda:///functionName');
   // @ts-ignore see "Why @ts-ignore" note
-  expect(Sentry.fakeScope.setContext).toBeCalledWith('runtime', { name: 'node', version: expect.anything() });
-  // @ts-ignore see "Why @ts-ignore" note
   expect(Sentry.fakeScope.setContext).toBeCalledWith(
     'aws.lambda',
     expect.objectContaining({
@@ -181,7 +179,7 @@ describe('AWSLambda', () => {
 
   describe('wrapHandler() on sync handler', () => {
     test('successful execution', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const handler: Handler = (_event, _context, callback) => {
         callback(null, 42);
@@ -201,7 +199,7 @@ describe('AWSLambda', () => {
     });
 
     test('unsuccessful execution', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const error = new Error('sorry');
       const handler: Handler = (_event, _context, callback) => {
@@ -273,7 +271,7 @@ describe('AWSLambda', () => {
     });
 
     test('capture error', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const error = new Error('wat');
       const handler: Handler = (_event, _context, _callback) => {
@@ -304,7 +302,7 @@ describe('AWSLambda', () => {
 
   describe('wrapHandler() on async handler', () => {
     test('successful execution', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const handler: Handler = async (_event, _context) => {
         return 42;
@@ -335,7 +333,7 @@ describe('AWSLambda', () => {
     });
 
     test('capture error', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const error = new Error('wat');
       const handler: Handler = async (_event, _context) => {
@@ -377,7 +375,7 @@ describe('AWSLambda', () => {
 
   describe('wrapHandler() on async handler with a callback method (aka incorrect usage)', () => {
     test('successful execution', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const handler: Handler = async (_event, _context, _callback) => {
         return 42;
@@ -408,7 +406,7 @@ describe('AWSLambda', () => {
     });
 
     test('capture error', async () => {
-      expect.assertions(10);
+      expect.assertions(9);
 
       const error = new Error('wat');
       const handler: Handler = async (_event, _context, _callback) => {
diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts
