chore(flamegraph/models): make it mandatory to handle all spyNames (#…

…1300) * chore(flamegraph/models): make it mandatory to handle all spyNames * chore(models): export spynames at runtime BREAKING CHANGE: it will throw an error if spyName is unsupported
grafana · Jul 25, 2022 · f7a95a0 · f7a95a0
1 parent 7088bb9
commit f7a95a0
Show file tree

Hide file tree

Showing 13 changed files with 78 additions and 28 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -109,6 +109,8 @@ module.exports = {
 
     // makes it easier to check what are local variables computated dynamically and what are static props
     'react/destructuring-assignment': 'off',
+
+    '@typescript-eslint/switch-exhaustiveness-check': 'error',
   },
   env: {
     browser: true,

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Flamegraph.ts b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Flamegraph.ts
@@ -5,6 +5,7 @@ import {
   Flamebearer,
   singleFF,
   doubleFF,
+  SpyName,
 } from '@pyroscope/models/src';
 import { PX_PER_LEVEL, BAR_HEIGHT, COLLAPSE_THRESHOLD } from './constants';
 import type { FlamegraphPalette } from './colorPalette';
@@ -78,7 +79,9 @@ export default class Flamegraph {
       sampleRate: this.flamebearer.sampleRate,
       names: this.flamebearer.names,
       levels: this.flamebearer.levels,
-      spyName: this.flamebearer.spyName,
+      // keep type narrow https://stackoverflow.com/q/54333982
+      // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
+      spyName: this.flamebearer.spyName as SpyName,
       units: this.flamebearer.units,
       maxSelf: this.flamebearer.maxSelf,
 

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Flamegraph_render.ts b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Flamegraph_render.ts
@@ -22,7 +22,7 @@ THIS SOFTWARE.
 */
 
 /* eslint-disable no-continue */
-import { createFF, Flamebearer } from '@pyroscope/models/src';
+import { createFF, Flamebearer, SpyName } from '@pyroscope/models/src';
 import {
   formatPercent,
   getFormatter,
@@ -240,7 +240,9 @@ export default function RenderCanvas(props: CanvasRendererConfig) {
           selectedLevel,
           highlightModeOn,
           isHighlighted,
-          spyName,
+          // keep type narrow https://stackoverflow.com/q/54333982
+          // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
+          spyName: spyName as SpyName,
           palette,
         };
 
@@ -368,7 +370,7 @@ type getColorCfg = {
   highlightModeOn: boolean;
   isHighlighted: boolean;
   names: string[];
-  spyName: string;
+  spyName: SpyName;
   palette: FlamegraphPalette;
 };
 

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/color.spec.ts b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/color.spec.ts
@@ -189,7 +189,7 @@ describe('getPackageNameFromStackTrace', () => {
       ],
     ])(`.getPackageNameFromStackTrace('%s')`, (a, expected) => {
       it(`returns '${expected}'`, () => {
-        expect(getPackageNameFromStackTrace('default', a)).toBe(expected);
+        expect(getPackageNameFromStackTrace('unknown', a)).toBe(expected);
       });
     });
   });

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/color.ts b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/color.ts
@@ -1,6 +1,7 @@
 /* eslint-disable camelcase */
 import Color from 'color';
 import { scaleLinear } from 'd3-scale';
+import type { SpyName } from '@pyroscope/models/src';
 import murmurhash3_32_gc from './murmur3';
 import type { FlamegraphPalette } from './colorPalette';
 
@@ -71,8 +72,8 @@ export function colorGreyscale(v: number, a: number) {
   return Color.rgb(v, v, v).alpha(a);
 }
 
-// TODO use @pyroscope/models?
-function spyToRegex(spyName: string) {
+function spyToRegex(spyName: SpyName): RegExp {
+  // eslint-disable-next-line default-case
   switch (spyName) {
     case 'dotnetspy':
       return /^(?<packageName>.+)\.(.+)\.(.+)\(.*\)$/;
@@ -100,16 +101,16 @@ function spyToRegex(spyName: string) {
       return /^(?<packageName>.+\/)(?<filename>.+\.)(?<functionName>.+)$/;
     case 'pyroscope-rs':
       return /^(?<packageName>[^::]+)/;
-
-    // we don't have enough information
-    default:
+    case 'unknown':
       return /^(?<packageName>.+)$/;
   }
+
+  return /^(?<packageName>.+)$/;
 }
 
 // TODO spy names?
 export function getPackageNameFromStackTrace(
-  spyName: string,
+  spyName: SpyName,
   stackTrace: string
 ) {
   if (stackTrace.length === 0) {

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/testData.ts b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/testData.ts
@@ -5,7 +5,7 @@ const TestData = {
     numTicks: 0,
     sampleRate: 0,
     units: 'samples' as const,
-    spyName: '',
+    spyName: 'unknown' as const,
     format: 'single' as const,
     version: 0,
   },
@@ -35,7 +35,7 @@ const TestData = {
     units: 'samples' as const,
     fitMode: 'HEAD',
 
-    spyName: 'gospy',
+    spyName: 'gospy' as const,
     version: 1,
   },
   ComplexTree: {
@@ -360,7 +360,7 @@ const TestData = {
     ],
     numTicks: 178,
     maxSelf: 16,
-    spyName: 'gospy',
+    spyName: 'gospy' as const,
     sampleRate: 100,
     units: 'samples' as const,
     format: 'single' as const,
@@ -414,7 +414,7 @@ const TestData = {
     ],
     numTicks: 1978,
     maxSelf: 604,
-    spyName: 'gospy',
+    spyName: 'gospy' as const,
     sampleRate: 100,
     units: 'samples' as const,
     format: 'double' as const,

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx
@@ -53,7 +53,7 @@ function mountFlamebearer(p: { profile?: Profile; flamebearer?: Flamebearer }) {
     names: [],
     units: '',
     levels: [[]],
-    spyName: '',
+    spyName: 'unknown',
     numTicks: 0,
     sampleRate: 0,
     maxSelf: 0,

diff --git a/packages/pyroscope-models/src/flamebearer.ts b/packages/pyroscope-models/src/flamebearer.ts
@@ -1,3 +1,4 @@
+import { SpyName } from './spyName';
 /**
  * @deprecated one should use the Profile model
  */
@@ -31,14 +32,7 @@ export type Flamebearer = {
     | 'lock_nanoseconds'
     | 'trace_samples'
     | '';
-  spyName:
-    | 'dotneyspy'
-    | 'ebpfspy'
-    | 'gospy'
-    | 'phpspy'
-    | 'pyspy'
-    | 'rbspy'
-    | string;
+  spyName: SpyName;
   // format: 'double' | 'single';
   //  leftTicks?: number;
   //  rightTicks?: number;

diff --git a/packages/pyroscope-models/src/index.ts b/packages/pyroscope-models/src/index.ts
@@ -2,3 +2,4 @@ export * from './profile';
 export * from './flamebearer';
 export * from './trace';
 export * from './decode';
+export * from './spyName';
diff --git a/packages/pyroscope-models/src/profile.spec.ts b/packages/pyroscope-models/src/profile.spec.ts
@@ -6,7 +6,7 @@ describe('Profile', () => {
       const expected = {
         format: 'single',
         sampleRate: 100,
-        spyName: '',
+        spyName: 'unknown',
         units: '',
       };
 

diff --git a/packages/pyroscope-models/src/profile.ts b/packages/pyroscope-models/src/profile.ts
@@ -1,4 +1,5 @@
 import { z } from 'zod';
+import { SpyNameSchema } from './spyName';
 
 export const FlamebearerSchema = z.object({
   names: z.array(z.string().nonempty()),
@@ -40,9 +41,7 @@ export const MetadataSchema = z.object({
 
   format: z.enum(['single', 'double']),
   sampleRate: z.number(),
-  spyName: z
-    .enum(['dotnetspy', 'ebpfspy', 'gospy', 'phpspy', 'pyspy', 'rbspy'])
-    .or(z.string()),
+  spyName: SpyNameSchema,
 
   units: UnitsSchema,
 });

diff --git a/packages/pyroscope-models/src/spyName.spec.ts b/packages/pyroscope-models/src/spyName.spec.ts
@@ -0,0 +1,18 @@
+import { SpyNameSchema, AllSpyNames } from './spyName';
+
+describe('SpyName', () => {
+  it('fails when when passing an unsupported value', () => {
+    expect(SpyNameSchema.safeParse('foo').success).toBe(false);
+  });
+
+  it('defaults to "unknown" when absent', () => {
+    expect(SpyNameSchema.parse('')).toBe('unknown');
+  });
+
+  test.each(AllSpyNames.map((a) => [a, a]))(
+    'parse("%s") should return "%s"',
+    (spyName) => {
+      expect(SpyNameSchema.parse(spyName)).toBe(spyName);
+    }
+  );
+});
diff --git a/packages/pyroscope-models/src/spyName.ts b/packages/pyroscope-models/src/spyName.ts
@@ -0,0 +1,30 @@
+import { z } from 'zod';
+
+export const SpyNameFirstClass = [
+  'dotnetspy',
+  'ebpfspy',
+  'gospy',
+  'phpspy',
+  'pyspy',
+  'rbspy',
+  'nodespy',
+  'javaspy',
+  'pyroscope-rs',
+] as const;
+
+export const SpyNameOther = [
+  'scrape', // for compability purposes, it should be golang
+  'tracing',
+  'unknown',
+] as const;
+
+export const AllSpyNames = [...SpyNameFirstClass, ...SpyNameOther] as const;
+
+export const SpyNameSchema = z.preprocess((val) => {
+  if (!val) {
+    return 'unknown';
+  }
+  return val;
+}, z.enum(AllSpyNames).default('unknown'));
+
+export type SpyName = z.infer<typeof SpyNameSchema>;