Skip to content

Conversation

@wrn14897
Copy link
Member

@wrn14897 wrn14897 commented Oct 14, 2025

Summary

Migrates the HyperDX API and related services from Winston to Pino for standardized,
faster, and more structured logging with improved OpenTelemetry integration.

Changes

Core Migration

  • Replaced Winston with Pino across all logging infrastructure
  • Upgraded @hyperdx/node-opentelemetry from v0.8.2 to v0.9.0 to support Pino
    transport
  • Removed deprecated dependencies:
    • winston and express-winston
    • @opentelemetry/host-metrics and @opentelemetry/sdk-metrics (consolidated into
      newer OTel SDK)
  • Added new dependencies:
    • pino and pino-http for core logging
    • pino-pretty for development console output

Logger Configuration (packages/api/src/utils/logger.ts)

Production:

  • Outputs stringified JSON to stdout via pino/file transport
  • Maintains HyperDX transport integration when API key is configured
  • Includes full OpenTelemetry trace context (trace_id, span_id, trace_flags)
Screenshot 2025-10-14 at 4 31 36 PM

Development:

  • Uses pino-pretty for human-readable, colorized console output
  • Hides verbose fields from console: pid, hostname, trace_id, span_id, trace_flags
  • HTTP request/response objects excluded from logs via custom serializers
image

HTTP Logging:

  • Replaced express-winston with pino-http
  • Custom log levels based on HTTP status codes (warn for 4xx, error for 5xx+)
  • Simplified log messages: HTTP {method} {url}

Error Logging Updates

Updated error logging patterns throughout the codebase to follow Pino's structured
logging conventions:

// Before (Winston)
logger.error('Error message:', error);

// After (Pino)
logger.error({ err: error }, 'Error message');

Ref: HDX-2588

This PR should also address issue: #1035

@changeset-bot
Copy link

changeset-bot bot commented Oct 14, 2025

🦋 Changeset detected

Latest commit: 1a4be4a

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/api Minor
@hyperdx/app Minor

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 Oct 14, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 15, 2025 4:34pm

},
});
} else {
targets.push({
Copy link
Member Author

Choose a reason for hiding this comment

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

In non-dev environments, the logger outputs a JSON string to stdout

@claude
Copy link

claude bot commented Oct 14, 2025

Pull Request Review: Migration from Winston to Pino

This PR migrates the HyperDX API logging infrastructure from Winston to Pino. Overall, this is a well-executed migration with good attention to detail.

Strengths

  1. Comprehensive Migration - All Winston references properly replaced with Pino equivalents with consistent structured logging pattern
  2. Smart Environment-Aware Configuration - Intelligently handles dev/CI/production environments with appropriate transports
  3. Improved HTTP Logging - Custom log levels based on HTTP status codes with cleaner messages
  4. OpenTelemetry Integration - Properly uses getPinoMixinFunction for trace context injection

Issues and Concerns

1. Critical: Type Safety Issue in logger.ts

Location: packages/api/src/utils/logger.ts:21

Using any[] defeats TypeScript type safety. Recommendation: Use proper typing from Pino (TransportTargetOptions[])

2. Potential Bug: Transport Configuration Logic

Location: packages/api/src/utils/logger.ts:47-54

The targets.length === 0 case should never happen but if it does, returning undefined will cause fallback to default console logging. Either remove the check or add a fallback default transport.

3. Missing Error Handling in usageStats.ts

Location: packages/api/src/tasks/usageStats.ts:162-164

Silently ignoring errors is acceptable but should be logged at debug level for troubleshooting.

4. Inconsistent Error Logging Pattern

Location: packages/api/src/routers/api/root.ts:201

Variable e is not typed and might not be an Error object. Use serializeError consistently.

5. Missing Tests

No tests were added to verify the new Pino logger configuration works correctly across different environments.

Recommendation: Add unit tests for logger initialization, transport selection logic, structured logging format, and error serialization.

Security Review

No Security Issues Detected - No credentials exposed, proper error serialization, no injection vulnerabilities introduced.

Performance Analysis

Positive Impact - Pino is significantly faster than Winston (up to 10x in benchmarks). Structured JSON logging is more efficient. Implementation properly optimizes for single vs multiple transports.

Code Quality

Generally follows CLAUDE.md guidelines with proper TypeScript typing (except any[] issue), consistent error handling patterns, and clear code organization.

Minor style issues: unused err parameter in customErrorMessage function, inconsistent error variable naming (e vs err).

Final Recommendations

Must Fix Before Merge

  1. Fix type safety: Replace any[] with proper Pino types in logger.ts:21
  2. Add basic tests: At minimum, add smoke tests verifying logger initialization

Should Fix (High Priority)

  1. Add error logging to usageStats.ts catch block
  2. Use serializeError consistently for error logging
  3. Remove unused err parameter in customErrorMessage

Nice to Have (Low Priority)

  1. Add comprehensive logger tests
  2. Document the migration if relevant
  3. Consider TypeScript strict mode checks for error types

Conclusion

This is a high-quality migration that follows best practices and properly implements structured logging with Pino. The code is clean, well-organized, and demonstrates good understanding of both ecosystems.

Recommendation: Approve with minor changes - Fix the type safety issue and add basic tests, then ready to merge.

Estimated Risk: Low - Changes are well-contained, backwards compatible in log output, and migration pattern is consistent throughout.

Great work!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2025

E2E Test Results

All tests passed • 25 passed • 3 skipped • 226s

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

View full report →

@wrn14897 wrn14897 changed the title migration: migrate to Pino migration: migrate to Pino (logger) Oct 15, 2025
@claude
Copy link

claude bot commented Oct 15, 2025

Pull Request Review - Winston to Pino Migration

Thank you for this comprehensive migration! This is a solid improvement to the logging infrastructure.

Strengths

Well-Structured Migration

  • Consistent conversion of Winston patterns to Pino's structured logging format
  • Proper use of Pino's err convention for error logging throughout
  • Good separation of development vs. production logging configurations

Improved Logging Configuration

  • Excellent use of pino-pretty for development with appropriate field hiding
  • Clean transport configuration with proper conditional logic
  • Maintains HyperDX integration via updated node-opentelemetry v0.9.0

HTTP Logging Enhancements

  • Smart customLogLevel logic (warn for 4xx, error for 5xx+)
  • Clean, concise HTTP log messages
  • Proper custom properties for user context

Consistent Error Patterns

  • All error logs properly converted to structured format
  • Good use of serializeError for error normalization

Issues and Recommendations

1. Type Safety Issue in logger.ts (Line 21)
Using any[] defeats TypeScript's type safety. Recommend importing TransportTargetOptions from pino and using that type.

2. Potential Configuration Edge Case
When no HyperDX API key is configured AND not in dev/CI mode, targets.length would be 0, resulting in transport: undefined. This means no logging output. Recommend always providing a default transport (stdout) for production.

3. Missing Error Type in customLogLevel
The err parameter has implicit any type. Add explicit typing for better type safety.

4. Test Coverage Consideration
The PR doesn't show test file changes. Were existing tests updated to work with Pino? Some integration tests might be mocking or asserting on Winston-specific behavior.

5. Usage Stats Logger (usageStats.ts)
This creates its own separate logger instance with a hardcoded API key. Is this intentional for tracking OSS usage separately?

Security Review

No security concerns identified:

  • Proper SSRF validation still in place
  • No sensitive data in log messages
  • API keys properly handled via environment variables
  • serializeError prevents accidental logging of sensitive error properties

Performance Considerations

Performance improvements expected:

  • Pino is significantly faster than Winston (5-10x improvement)
  • Structured JSON logging is more efficient
  • Proper use of transports avoids blocking the event loop

Testing Recommendations

Before merging, please verify:

  1. All existing tests pass with Pino
  2. Logs appear correctly in production format (JSON to stdout)
  3. Logs appear correctly in dev format (pino-pretty colorized)
  4. HyperDX integration still works
  5. HTTP request logging includes all expected fields
  6. Error stack traces are properly captured

Summary

This is a high-quality migration with consistent patterns throughout. The main concerns are type safety improvements and edge case handling in logger.ts, plus test coverage verification. The migration properly addresses issue 1035 and will provide better logging performance.

Recommendation: Approve with minor changes - address the type safety issues and edge case handling, then this is ready to merge.

pulpdrew
pulpdrew previously approved these changes Oct 15, 2025
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 with one suggestion -

There are a few places throughout the codebase that call logger.info({ message: '...' }). In winston, that made sense since winston treated the message field as significant. Pino on the other hand seems to use msg as the message field in logged objects. It might be good to be consistent and replace message with msg.

Example: https://github.com/hyperdxio/hyperdx/blob/main/packages/api/src/tasks/checkAlerts.ts#L96

const user = req.user;
if (user) {
return {
userId: user._id?.toString(),
Copy link
Member Author

Choose a reason for hiding this comment

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

likesetTraceAttributes (

setTraceAttributes({
userId: req.user?._id.toString(),
userEmail: req.user?.email,
});
), we also want to add the same attributes for logs

@wrn14897
Copy link
Member Author

LGTM with one suggestion -

There are a few places throughout the codebase that call logger.info({ message: '...' }). In winston, that made sense since winston treated the message field as significant. Pino on the other hand seems to use msg as the message field in logged objects. It might be good to be consistent and replace message with msg.

Example: main/packages/api/src/tasks/checkAlerts.ts#L96

Good call! Done

@kodiakhq kodiakhq bot merged commit 348a404 into main Oct 15, 2025
10 checks passed
@kodiakhq kodiakhq bot deleted the warren/migrate-to-pino branch October 15, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants