refactor: unify source types and remove ambiguity #1194

knudtty · 2025-09-22T20:21:31Z

Closes HDX-2468

changeset-bot · 2025-09-22T20:21:35Z

⚠️ No Changeset found

Latest commit: 2108f6c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2025-09-22T20:21:36Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
hyperdx-v2-oss-app	Ready	Preview	Comment	Sep 23, 2025 4:02pm

github-actions · 2025-09-22T21:22:32Z

E2E Test Results

✅ All tests passed • 21 passed • 3 skipped • 151s

Status	Count
✅ Passed	21
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	3

View full report →

knudtty · 2025-09-22T23:28:47Z

packages/api/src/controllers/sources.ts

 export function createSource(team: string, source: Omit<ISource, 'id'>) {
-  return Source.create({ ...source, team });
+  switch (source.kind) {
+    case SourceKind.Log:
+      return LogSource.create({ ...source, team });
+    case SourceKind.Trace:
+      return TraceSource.create({ ...source, team });
+    case SourceKind.Metric:
+      return MetricSource.create({ ...source, team });
+    case SourceKind.Session:
+      return SessionSource.create({ ...source, team });
+  }


Mongoose will strip out any unnecessary fields, so create source must use the new broken out discriminated models

knudtty · 2025-09-22T23:29:01Z

packages/api/src/controllers/sources.ts

+  switch (source.kind) {
+    case SourceKind.Log:
+      return LogSource.findOneAndUpdate({ _id: sourceId, team }, source, {
+        new: true,
+      });
+    case SourceKind.Trace:
+      return TraceSource.findOneAndUpdate({ _id: sourceId, team }, source, {
+        new: true,
+      });
+    case SourceKind.Metric:
+      return MetricSource.findOneAndUpdate({ _id: sourceId, team }, source, {
+        new: true,
+      });
+    case SourceKind.Session:
+      return SessionSource.findOneAndUpdate({ _id: sourceId, team }, source, {
+        new: true,
+      });
+  }


Same, using discriminated models

knudtty · 2025-09-22T23:30:01Z

packages/api/src/models/source.ts

-      durationPrecision: Number,
-      parentSpanIdExpression: String,
-      spanNameExpression: String,
+export const LogSource = Source.discriminator<


Broke out the mongoose models into the base model (Source) and a discriminated model for each type

knudtty · 2025-09-22T23:30:36Z

packages/api/src/routers/api/__tests__/sources.test.ts

@@ -1,10 +1,10 @@
-import { SourceKind, TSourceUnion } from '@hyperdx/common-utils/dist/types';
+import { SourceKind, TSource } from '@hyperdx/common-utils/dist/types';


TSourceUnion is renamed TSource

knudtty · 2025-09-22T23:34:02Z

packages/app/src/source.ts

+export function useSource(params: {
+  id?: string | null;
+  connection?: string | null;
+}): UseQueryResult<TSource | undefined>;
+export function useSource<K extends TSource['kind']>(params: {
+  id?: string | null;
+  connection?: string | null;
+  kind: K;
+}): UseQueryResult<Extract<TSource, { kind: K }> | undefined>;


The type signature without specifying kind returns the TSource type, but if you specify kind then the type signature understands the returned source is a particular kind. Super useful if you always know you'll be using a specific kind of source

knudtty · 2025-09-22T23:35:04Z

packages/app/src/source.ts

-    mutationFn: async ({ source }: { source: Omit<TSource, 'id'> }) => {
+    mutationFn: async ({ source }: { source: TSourceNoId }) => {


TS's built in Omit does not work well with discriminated unions, so it's better to construct with zod and type assert.

wrn14897 · 2025-09-23T05:08:15Z

The refactoring makes me a bit nervous, as the changes affect the core of the entire app and I’m not very confident in our test coverage in this area. To minimize the risk of breaking existing behavior, I’d suggest migrating to the new sources model gradually, one source type at a time. If it’s not that easy, I’d recommend adding more unit tests for different sources.

Overall, I really like these changes, which will help us catch more source type–related bugs :)

wrn14897 · 2025-09-23T05:09:13Z

packages/api/src/controllers/sources.ts

+      return LogSource.create({ ...source, team });
+    case SourceKind.Trace:
+      return TraceSource.create({ ...source, team });
+    case SourceKind.Metric:
+      return MetricSource.create({ ...source, team });
+    case SourceKind.Session:
+      return SessionSource.create({ ...source, team });


we should also handle default case

wrn14897 · 2025-09-23T05:11:17Z

packages/api/src/models/source.ts

+export const SessionSource = Source.discriminator<
+  Extract<TSource, { kind: SourceKind.Session }>
+>(
+  SourceKind.Session,
+  new mongoose.Schema<Extract<TSource, { kind: SourceKind.Session }>>({
+    traceSourceId: String,
+  }),
 );


Will those non-public fields (e.g., ResourceAttributes) be inserted or updated?

Let's talk about the requirements for Sessions, because ResourceAttributes isn't currently listed in the schema

Hmm you are right. Sorry, I was thinking about the different thing. The mapping should only require a few fields

wrn14897 · 2025-09-23T05:12:38Z

packages/api/src/routers/api/sources.ts

+        case SourceKind.Log:
+          return s.toJSON({ getters: true });
+        case SourceKind.Trace:
+          return s.toJSON({ getters: true });
+        case SourceKind.Metric:
+          return s.toJSON({ getters: true });
+        case SourceKind.Session:
+          return s.toJSON({ getters: true });


nit: this can be

case SourceKind.Log: case SourceKind.Trace: case SourceKind.Metric: case SourceKind.Session: return s.toJSON({ getters: true });

also we need to handle default case

For some reason Typescript gets confused about the union so that does not work, it has to be broken out like this. I'll add an error for the default case

wrn14897 · 2025-09-23T05:21:17Z

packages/app/src/DBSearchPage.tsx

+  const { data: logSource, isLoading: isLogSourceLoading } = useSource({
+    id: source,
+    kind: SourceKind.Log,
+  });
+  const { data: traceSource, isLoading: isTraceSourceLoading } = useSource({
    id: source,
+    kind: SourceKind.Trace,
  });


Is there any chance we could avoid duplicate requests here? I see the point, but the source ID should already give us the right source

Under the hood they all are using the same query key to fetch all the sources and then just running find over the elements of the array. They are using the same query key, so only one request should be generated

knudtty · 2025-09-23T13:46:50Z

To minimize the risk of breaking existing behavior, I’d suggest migrating to the new sources model gradually, one source type at a time. If it’s not that easy, I’d recommend adding more unit tests for different sources.

Unfortunately it is not easy. More unit tests makes sense, I'll seek guidance for which ones specifically we are interested in

unify source types and remove ambiguity

23d5fde

vercel bot deployed to Preview September 22, 2025 20:24 View deployment

Merge branch 'main' into aaron/source-tech-debt

5b3ed93

vercel bot deployed to Preview September 22, 2025 21:22 View deployment

fix linting

e25a223

vercel bot deployed to Preview September 22, 2025 21:44 View deployment

fix: sources not searching properly

0d65ec6

vercel bot deployed to Preview September 22, 2025 23:21 View deployment

knudtty requested a review from wrn14897 September 22, 2025 23:24

knudtty commented Sep 22, 2025

View reviewed changes

wrn14897 reviewed Sep 23, 2025

View reviewed changes

add defaults to source switches

2108f6c

vercel bot deployed to Preview September 23, 2025 16:02 View deployment

		@@ -1,10 +1,10 @@
		import { SourceKind, TSourceUnion } from '@hyperdx/common-utils/dist/types';
		import { SourceKind, TSource } from '@hyperdx/common-utils/dist/types';

		mutationFn: async ({ source }: { source: Omit<TSource, 'id'> }) => {
		mutationFn: async ({ source }: { source: TSourceNoId }) => {

refactor: unify source types and remove ambiguity #1194

Are you sure you want to change the base?

refactor: unify source types and remove ambiguity #1194

Uh oh!

Conversation

knudtty commented Sep 22, 2025

Uh oh!

changeset-bot bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercel bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results

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!

wrn14897 commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

knudtty commented Sep 23, 2025

Uh oh!

Uh oh!

changeset-bot bot commented Sep 22, 2025 •

edited

Loading

vercel bot commented Sep 22, 2025 •

edited

Loading

github-actions bot commented Sep 22, 2025 •

edited

Loading

wrn14897 commented Sep 23, 2025 •

edited

Loading