chore: split json otel collector to enable both during dev

[knudtty]

Gets us closer to a staging instance of json

[changeset-bot]

⚠️ No Changeset found

Latest commit: cea9672

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]

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	Oct 9, 2025 4:01pm

[github-actions]

E2E Test Results

✅ All tests passed • 25 passed • 3 skipped • 195s

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

View full report →

[claude]

PR Review: Split JSON OTel Collector for Dual Dev Environment

Summary

This PR adds support for running two OpenTelemetry collectors simultaneously in development: one with the standard schema and one with the JSON schema enabled. This allows testing both configurations side-by-side.

---

✅ Strengths

1. Clear separation of concerns: The PR cleanly separates the JSON and standard OTel collector configurations
2. Non-breaking changes: The changes are dev-only and don't affect production behavior
3. Port mapping strategy: Good use of distinct ports (13134, 14317, 14318, 18888) to avoid conflicts

---

🐛 Potential Issues

1. Logic Error in server.ts:46 (Critical)

if (!config.IS_DEV || !config.IS_OPAMP_ONLY) {

This condition has a logical flaw. The app server will start in production OR when IS_OPAMP_ONLY=false in dev. However, the intended behavior seems to be:

• In production: always start the app server
• In dev with IS_OPAMP_ONLY=true: skip the app server
• Otherwise: start the app server

Recommended fix:

if (!config.IS_OPAMP_ONLY) {
  // Start app server
}

Or more explicitly:

if (config.IS_PROD || (config.IS_DEV && !config.IS_OPAMP_ONLY)) {
  // Start app server
}

The current logic would incorrectly skip the app server in production if IS_OPAMP_ONLY=true were somehow set.

2. Missing Error Handling for Empty apiKeys Array (Medium)

In opampController.ts:160, when apiKeys.length > 0 is false but we still try to access apiKeys[0], it returns undefined:

authorization: apiKeys.length > 0 ? apiKeys[0] : '',

This is handled, but it means the JSON collector will forward data with an empty authorization header, which could fail. Consider logging a warning when this occurs in dev mode.

3. Hardcoded Port in opampController.ts:158 (Low)

endpoint: 'http://host.docker.internal:14318',

This hardcodes port 14318. While this matches the docker-compose configuration, it would be more maintainable to use a config variable or at least add a comment explaining the relationship.

---

🔒 Security Considerations

1. Dev-only guards are present: The JSON schema logic is properly gated with config.IS_DEV checks ✅
2. API key handling: API keys are passed through the forwarding configuration, which is appropriate for dev environments ✅
3. No exposed secrets: Configuration uses environment variables appropriately ✅

---

⚡ Performance Considerations

1. Development overhead: Running two OTel collectors will consume approximately 2x the resources (CPU, memory). This is acceptable for dev but should be documented.
2. Data duplication: The non-JSON collector forwards to the JSON collector, which means data flows through two collectors. This is intentional for testing but worth noting.
3. Docker network: Both collectors depend on ch-server with health checks, which is good for preventing startup race conditions ✅

---

🧪 Test Coverage

Concerns:

• No tests found for opampController.ts or server.ts
• The new conditional logic in buildOtelCollectorConfig() and server.start() is untested
• Configuration variations should have unit tests to prevent regressions

Recommendations:

1. Add unit tests for buildOtelCollectorConfig() with different IS_DEV, IS_JSON_OPAMP combinations
2. Add tests for the server startup logic with IS_OPAMP_ONLY variations
3. Consider integration tests that verify both collectors can start simultaneously

---

📝 Code Quality & Style

Consistency with CLAUDE.md Guidelines

1. TypeScript typing: Good use of typed configurations ✅
2. Code organization: Changes are properly localized to relevant files ✅
3. Variable naming: Clear and descriptive variable names ✅

Minor Issues:

1. Inconsistent object spread pattern (opampController.ts:263):

...(otlpExporter ? otlpExporter : {})

Could be simplified to:

...(otlpExporter['otlphttp/internal'] ? otlpExporter : {})

Or even better, avoid the conditional entirely since spreading { 'otlphttp/internal': undefined } is harmless.

2. Variable initialization (opampController.ts:141-147):

let otlpExporter: {...} = {
  'otlphttp/internal': undefined,
};

Initializing to undefined and then conditionally setting it is fine, but consider initializing as an empty object and only adding the property when needed:

let otlpExporter: Partial<CollectorConfig['exporters']> = {};
if (!config.IS_JSON_OPAMP) {
  otlpExporter['otlphttp/internal'] = { /* ... */ };
}

3. Missing type safety (opampController.ts:271, 282):
   Arrays are spread with fallback to empty array:

...(otlpForward ? otlpForward : [])

This can be simplified to:

...(otlpForward || [])

---

📚 Documentation Gaps

1. No inline comments explaining the data flow between the two collectors
2. Package.json script adds JSON-OPAMP to concurrently but doesn't document what it does
3. Docker compose comment on line 69-70 references BETA_CH_OTEL_JSON_SCHEMA_ENABLED but doesn't explain the relationship to the JSON collector

Recommendation: Add a comment block in opampController.ts explaining the dev-mode dual-collector architecture.

---

✨ Suggestions for Improvement

1. Extract magic numbers/strings:

const JSON_COLLECTOR_PORTS = {
  OTLP_HTTP: 14318,
  OPAMP: 4321,
};

2. Add validation: Ensure IS_JSON_OPAMP and IS_OPAMP_ONLY are mutually exclusive (or document why they can both be true)

3. Environment variable defaults: Consider adding defaults for OPAMP_PORT in case it's not set:

export const OPAMP_PORT = Number.parseInt(env.OPAMP_PORT as string) || 8080;

---

🎯 Summary & Recommendation

Overall Assessment: ✅ Approve with minor changes

This is a solid PR that enables an important development capability. The changes are well-scoped and appropriately guarded for dev-only usage.

Required Changes:

1. Fix the logical condition in server.ts:46

Recommended Changes:

1. Add unit tests for the new configuration logic
2. Add inline documentation explaining the dual-collector architecture
3. Consider extracting hardcoded ports to constants
4. Add warning log when forwarding with empty API key

Nice to Have:

1. Simplify conditional spreads
2. Add environment variable validation/defaults

Great work on this feature! The dual-collector setup will be very useful for testing JSON schema migration. 🚀