Disable xray tracing (middleware) #3323
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/core/tracing/tracing.module.ts (1)
Line range hint
16-24: Review the complete XRay integration architecture.While the middleware application is disabled, the module still:
- Initializes XRay configuration in
onModuleInit- Registers XRay-related providers
- Maintains the XRayMiddleware as a global interceptor
This partial disablement might still incur performance overhead and could complicate the future transition to OpenTelemetry.
Consider:
- Creating a feature flag in ConfigService to completely disable XRay integration
- Conditionally registering providers based on this flag
- Creating a separate module for the planned OpenTelemetry integration
Example provider registration with feature flag:
@Module({ imports: [LoggerModule], providers: [ TracingService, { provide: APP_INTERCEPTOR, useFactory: (config: ConfigService) => { return config.xray.enabled ? new XRayMiddleware() : null; }, inject: [ConfigService], }, // ... other providers ], })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/core/tracing/tracing.module.ts(1 hunks)
🔇 Additional comments (1)
src/core/tracing/tracing.module.ts (1)
34-35: Verify the impact of disabling XRay tracing.
Let's ensure there are no direct dependencies on XRay tracing in the codebase.
✅ Verification successful
Disabling XRay tracing is safe but requires configuration awareness
The code analysis reveals that XRay tracing is an optional feature that's already properly handled:
- The middleware is already safely disabled through commenting out
consumer.apply(XRayMiddleware).forRoutes('*') - The implementation includes a safety check: tracing is automatically disabled when no daemon address is configured (
if (!this.config.xray.daemonAddress)) - The XRay setup is properly isolated in the tracing module with no direct dependencies from other parts of the application
- The trace context is only used for AWS XRay specific functionality and doesn't affect other application logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for XRay middleware usage and dependencies
# Check for direct XRay middleware usage
echo "Checking for XRay middleware usage..."
rg -l "XRayMiddleware" --type ts
# Check for direct segment access
echo "Checking for direct XRay segment access..."
rg -l "XRay\.getSegment" --type ts
# Check for trace ID usage
echo "Checking for trace ID usage..."
rg -l "traceId" --type ts
Length of output: 417
Script:
#!/bin/bash
# Let's examine the XRay middleware implementation and its usage
# Check the content of XRay middleware
echo "=== XRay Middleware Implementation ==="
cat src/core/tracing/xray.middleware.ts
# Check the full context of tracing module
echo -e "\n=== Tracing Module Implementation ==="
cat src/core/tracing/tracing.module.ts
# Look for any AWS XRay imports or configurations
echo -e "\n=== AWS XRay Related Imports ==="
rg -l "aws-xray" --type ts
# Check for any trace context or correlation ID patterns
echo -e "\n=== Trace Context Usage ==="
rg "(?i)(trace|correlation).*id" --type ts
Length of output: 8144
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/core/tracing/xray.middleware.ts (1)
Line range hint
16-121: Consider documenting migration plans and cleaning up unused code.While disabling the XRay tracing addresses the immediate performance concerns, consider the following improvements:
- Add TODO comments about the planned OpenTelemetry migration
- Document the current disabled state in the class JSDoc
- Consider moving the XRay implementation to a separate file/folder to prepare for future removal once OpenTelemetry is implemented
This will help maintain clarity about the current state and future plans.
Would you like me to provide a code snippet with the suggested documentation improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/core/tracing/xray.middleware.ts(2 hunks)
🔇 Additional comments (1)
src/core/tracing/xray.middleware.ts (1)
9-9: LGTM: Import statement is correctly placed.
The import of the Disabled decorator is properly organized with other imports.
It turns out to be not so useful in its current state.
There is a performance hit to run this logic so disabling seems appropriate.
I would like to look at OpenTelemetry next.
It seems to be the actual standard, has better support, and AWS integrates with it (in a different way), and can provide a better UI via the standardized ingress.
Summary by CodeRabbit
New Features
Bug Fixes