Skip to content

fix: better source validation and refine required source fields#1895

Open
knudtty wants to merge 18 commits intomainfrom
aaron/sources-but-better
Open

fix: better source validation and refine required source fields#1895
knudtty wants to merge 18 commits intomainfrom
aaron/sources-but-better

Conversation

@knudtty
Copy link
Contributor

@knudtty knudtty commented Mar 12, 2026

Summary

Large refactor changing the TSource type to a true discriminated union. This means that the expected fields for kind: 'log' will differ from those for 'trace', 'session', 'metrics'. This avoids the current laissez faire source type that currently exists, and required extensive changes across the api and app packages. Also includes a nice addition to useSource - you can now specify a kind field, which will properly infer the type of the returned source.

This also makes use of discriminators in mongoose. This does change a bit of the way that we create and update sources. Obvious changes to sources have also been made, namely making timeValueExpression required on sources. Care has been taken to avoid requiring a migration.

How to test locally or on Vercel

  1. yarn dev
  2. Play around with the app, especially around source creation, source edits, and loading existing sources from a previous version

References

  • Linear Issue: References HDX-3352
  • Related PRs:

Ref: HDX-3352

@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2026

🦋 Changeset detected

Latest commit: d73d46d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Mar 12, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 18, 2026 7:38pm

Request Review

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

PR Review

  • Behavioral regression in RawSqlChartEditor.tsx: isMetricSource(source) replaced source.kind !== SourceKind.Metric — these are opposite conditions. Log/Trace source base tables are now excluded from the SQL editor's table list instead of included. → Revert to !isMetricSource(source).

  • createdAt lost on kind-change: updateSource() does a raw Source.collection.replaceOne(…) that includes updatedAt but omits createdAt, so the timestamp is permanently dropped from documents whose kind changes. → Add createdAt: existing.createdAt to the replacement object.

  • ⚠️ Breaking change for Trace sources missing defaultTableSelectExpression: New schema makes this field required (z.string().min(1)), but existing Trace sources without it will fail the internal PUT route and be filtered out of external API responses (via safeParse failure). No applyLegacyDefaults() equivalent exists for Trace unlike Session. → Either add a migration/default or add applyLegacyDefaults handling for Trace sources.

  • ⚠️ Notification flooding on load: useSources fires one notifications.show (with autoClose: false) per invalid source inside the query function. If multiple sources fail validation post-deploy, the user sees an uncleared flood of toasts. → Batch into a single notification or limit to one.

  • ⚠️ sessionSourceId silently removed from log source: OnboardingModal.tsx no longer sets sessionSourceId on the log source update. If any code still reads logSource.sessionSourceId for correlation, it will silently break. → Verify sessionSourceId is fully removed from all consumers, or document the intentional removal.

  • ⚠️ @ts-expect-error on discriminator create/findOneAndUpdate: Suppresses a real type mismatch between DistributiveOmit<ISourceInput, 'id'> and the specific per-kind Mongoose model create signature. Runtime coercions could silently drop kind-specific required fields. → Investigate and fix the underlying type rather than suppressing.

  • ⚠️ Session table schema validation removed with no replacement: The useEffect that validated session table schema compatibility in SourceForm.tsx was fully removed. Users configuring a session source get no feedback until submit failure. → Restore equivalent validation or add inline field-level feedback.

  • ℹ️ formatExternalSource switch is all-identical branches (packages/api/src/routers/external-api/v2/sources.ts): All four case arms call source.toJSON({ getters: true }) identically. → Remove the switch and call toJSON directly, or add a comment explaining future intent.

  • ℹ️ OpenAPI spec marks Session.timestampValueExpression as nullable: true but BaseSourceSchema now requires it (.min(1)), and applyLegacyDefaults() backfills it. The spec is now incorrect. → Update the OpenAPI annotation to required: true.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

E2E Test Results

All tests passed • 91 passed • 3 skipped • 979s

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

Tests ran across 4 shards in parallel.

View full report →

@knudtty knudtty requested review from a team and pulpdrew and removed request for a team March 18, 2026 03:22
Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

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

Massive improvement! Just a couple questions and minor suggestions

Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants