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: f6dedd2

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 15, 2025 8:25pm

[github-actions]

E2E Test Results

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

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

View full report →

[claude]

Pull Request Review: Split JSON OTel Collector for Dev Environment

This PR introduces a second OTel collector instance (otel-collector-json) to run alongside the standard collector during development.

Strengths

• Clear separation of concerns using feature flags
• Backward compatible with existing workflow
• Well-structured conditional logic

Critical Issues

1. Server Startup Logic Bug (HIGH PRIORITY)
Location: packages/api/src/server.ts:49
The condition !config.IS_DEV || !config.IS_OPAMP_ONLY will start the app server in production even when IS_OPAMP_ONLY=true. Should be !(config.IS_DEV && config.IS_OPAMP_ONLY).

2. Missing Tests (HIGH PRIORITY)
No unit tests for new config flags (IS_OPAMP_ONLY, IS_JSON_OPAMP), server startup logic combinations, or OpAMP controller conditional config.

3. Missing Documentation (MEDIUM PRIORITY)
New environment variables lack JSDoc comments explaining their purpose and usage.

Other Issues

OPAMP_PORT Validation (LOW)
Location: packages/api/src/config.ts:32
If OPAMP_PORT is not set, parseInt returns NaN. Add validation or default value.

Redundant Conditional (LOW)
Location: packages/api/src/opamp/controllers/opampController.ts:263
The spread ...(otlpExporter ? otlpExporter : {}) is redundant since otlpExporter is always defined.

Hardcoded Port (MEDIUM)
Location: docker-compose.dev.yml:65
JSON OpAMP URL uses port 4321 but default is 4320. Add comment explaining why.

API Key Handling (INFO)
Location: packages/api/src/opamp/controllers/opampController.ts:160
Consider logging warning when no API key available in dev mode.

Performance Note
Running two collectors doubles resource usage. Document when to use JSON vs standard collector.

Concurrency Colors (LOW)
package.json has 4 colors but 5 processes. Add 5th color.

Recommendations

Must Fix:

1. Fix boolean logic in server.ts:49
2. Add unit tests for new functionality
3. Document new environment variables

Should Fix:
4. Add OPAMP_PORT validation
5. Document resource implications
6. Add integration tests

Nice to Have:
7. Clean up redundant conditionals
8. Improve naming consistency
9. Fix concurrency script colors

Overall

Well-structured PR that achieves its goal. Main blockers are the logic bug and missing tests. Once addressed, ready to merge!

Reviewed per CLAUDE.md conventions

[knudtty]

@claude

[claude]

Pull Request Review

Summary

This PR adds support for running a separate JSON-enabled OpenTelemetry collector instance in development mode alongside the standard collector. This allows testing both JSON and non-JSON schemas simultaneously during development.

Code Quality & Best Practices ✅

Positive aspects:

• Clean separation of concerns with environment-based configuration
• Follows existing codebase patterns and conventions
• Proper TypeScript typing throughout
• Good use of conditional logic based on environment flags

Areas for improvement:

1. Logic Clarity in server.ts:49

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

This double-negative condition is confusing. Consider refactoring for readability:

// Start app server unless we're in dev mode running OPAMP only
const shouldStartAppServer = !(config.IS_DEV && config.IS_OPAMP_ONLY);
if (shouldStartAppServer) {
  // ...
}

2. Potential Issue: Server Property Access

In server.ts:49-60, this.appServer is only conditionally initialized. If the condition is false, this.appServer will be undefined, which could cause issues if any code tries to access it later. Consider initializing it as nullable:

protected appServer?: http.Server;  // Make it optional

3. Magic Number in opampController.ts:160

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

Silently using an empty string when no API keys exist could lead to authentication failures that are hard to debug. Consider adding a warning log or comment explaining this behavior.

4. Type Safety in opampController.ts:263

...(otlpExporter ? otlpExporter : {})

This can be simplified or better yet, initialize otlpExporter as an empty object and conditionally add the property.

Performance Considerations ⚡

• ✅ Good: Running two collectors in parallel during development is acceptable and won't impact production
• ⚠️ Minor concern: The forwarding setup (otlphttp/internal) adds network hops in dev mode. Ensure this doesn't cause significant latency during testing
• ✅ Good: The forwarding is only enabled in dev mode, so no production impact

Security Concerns 🔒

1. Development-Only Hardcoded Endpoint

In opampController.ts:158:

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

✅ Good: This is properly gated behind config.IS_DEV
⚠️ Suggestion: Add a comment explaining this is for local Docker dev forwarding to prevent confusion

2. API Key Handling

In opampController.ts:160:

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

⚠️ Concern: Using an empty string as fallback could mask configuration issues. Consider:

• Logging a warning if no API keys are available in dev mode
• Using a dev-specific API key constant when running in dev mode

3. Port Conflicts

The new otel-collector-json service uses remapped ports (13134, 24226, 14317, 14318, 18888). Ensure these ports are documented to avoid developer confusion when troubleshooting connection issues.

Potential Bugs 🐛

1. Uninitialized Server in Edge Cases

If config.IS_DEV && config.IS_OPAMP_ONLY is true, this.appServer won't be initialized but isn't marked as optional. This could cause runtime errors if any shutdown or cleanup code tries to access it.

Recommendation: Update the type definition:

protected appServer?: http.Server;  // Make optional
protected opampServer!: http.Server;  // This is always initialized

2. Missing Port Configuration Validation

The OPAMP_PORT is used directly without validation. If it's undefined or invalid, Number.parseInt will return NaN, causing the server to fail silently.

Recommendation: Add validation in config.ts

Test Coverage 🧪

Missing test coverage:

1. No tests for buildOtelCollectorConfig with the new JSON schema logic
2. No tests for the server startup conditional logic
3. No tests for the OTLP forwarding configuration

Recommendations:

1. Add unit tests for buildOtelCollectorConfig covering:

   • IS_JSON_OPAMP=true scenario (should use otel_logs_json tables)
   • IS_JSON_OPAMP=false scenario (should include OTLP forwarding)
   • Both paths in dev vs. production environments

2. Add integration tests to verify:

   • Both collectors can start simultaneously
   • Data flows correctly through the forwarding path
   • No port conflicts occur

3. Test the server startup logic with various combinations of IS_DEV and IS_OPAMP_ONLY

Documentation 📝

Missing:

1. No documentation in the PR description about how to use the new dev setup
2. No comments explaining the forwarding architecture
3. The new dev:json script isn't documented

Recommendations:

1. Update CLAUDE.md or relevant dev docs to explain:
   • When to use the JSON collector
   • How the forwarding works
   • What ports each collector uses
2. Add inline comments in opampController.ts explaining the forwarding logic

Additional Suggestions

Docker Compose Configuration

In docker-compose.dev.yml, consider:

1. Adding a comment header explaining the difference between otel-collector and otel-collector-json
2. Using a Docker network alias to make the forwarding endpoint more semantic

Environment Variables

Consider adding to .env.example or documentation:

• OPAMP_PORT=8090
• IS_OPAMP_ONLY=false
• IS_JSON_OPAMP=false

Verdict

Overall Assessment: This is a well-structured change that follows the codebase patterns. The main concerns are around:

• Edge case handling (uninitialized server)
• Test coverage
• Documentation
• Minor code clarity improvements

Recommendation: ✅ Approve with suggestions - The code is functional and safe for development use, but would benefit from the improvements listed above before merging.

Priority Issues to Address:

1. 🔴 High: Fix server property typing (appServer?: http.Server)
2. 🟡 Medium: Add test coverage for new configuration logic
3. 🟡 Medium: Add port validation for OPAMP_PORT
4. 🟢 Low: Improve code clarity in server startup condition
5. 🟢 Low: Add documentation for new dev workflow

[wrn14897]

I think adding OTEL_AGENT_FEATURE_GATE_ARG should be sufficient. We can let developers decide whether to switch over and test the JSON columns to minimize code changes. If we are going to test JSON features more, imo we should just enable it by default