fix: integrate sentry with tracing and axum #3290
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR Review: Integrate Sentry with Tracing and AxumI've reviewed this PR that upgrades Sentry integration and consolidates error reporting through the tracing layer. Overall, this is a solid improvement that modernizes the Sentry integration and reduces code duplication. Here are my findings: ✅ Strengths
🔍 Issues & ConcernsCritical: Missing Sentry Dependency in runtime packageLocation: The PR adds Action: Ensure Medium: Layer Ordering in Tracing SubscriberLocation: let registry = registry
.with(reload_layer)
.with(otel_metric_layer)
.with(otel_trace_layer)
.with(sentry::integrations::tracing::layer()) // ← Added here
.with(SentryOtelLayer);Concern: The Question: Does the Sentry layer capture events in Recommendation: Consider reordering to: .with(SentryOtelLayer) // Set trace context first
.with(sentry::integrations::tracing::layer()) // Then let Sentry layer captureOr verify through testing that the current order correctly propagates OTel trace IDs to Sentry. Medium: Scope Mutation Thread SafetyLocation: sentry::configure_scope(|scope| {
scope.set_context(
"trace",
sentry::protocol::TraceContext { /* ... */ },
);
});Concern:
Recommendation: Use sentry::with_scope(|scope| {
scope.set_context("trace", /* ... */);
// Capture within this scope if needed
}, || {});Actually, looking more closely, since this is in Low: Error Removal Without ReplacementLocations:
The manual Verification Needed: Ensure that the tracing layer automatically captures these errors. Based on the code:
But: The old code set explicit tags like Action: Test that error metadata (error codes, workflow IDs, HTTP status codes) appears in Sentry events after this change. 🎯 Recommendations1. Add Integration TestsSince there are no existing tests in runtime or api-builder packages, consider adding at least a smoke test: #[test]
fn test_sentry_otel_layer_trace_context() {
// Verify OTel trace IDs propagate to Sentry
}2. Document the IntegrationAdd a comment in /// Custom tracing layer that synchronizes OpenTelemetry trace IDs with Sentry.
/// This ensures that errors reported to Sentry are correlated with distributed
/// traces, enabling cross-system debugging.
struct SentryOtelLayer;3. Verify Sentry Feature FlagsThe new features added:
Good choices here. 4. Consider Error Level FilteringThe Sentry tracing layer might capture all error-level logs. Consider adding a filter if you only want specific errors: .with(
sentry::integrations::tracing::layer()
.event_filter(|metadata| {
metadata.level() >= &tracing::Level::ERROR
})
)🔐 Security Considerations✅ No issues found. The Sentry DSN is already in the codebase (telemetry/src/lib.rs:3), so no new secrets exposed. ⚡ Performance ConsiderationsMinor: The
This happens on every trace event. Consider:
Suggestion: fn on_event(&self, event: &tracing::Event<'_>, ctx: Context<'_, S>) {
// Early return for non-error events
if \!matches\!(event.metadata().level(), &tracing::Level::ERROR | &tracing::Level::WARN) {
return;
}
// Rest of implementation...
}📋 Test CoverageMissing: No tests were added for the new Recommendation: Add tests or manually verify:
✅ SummaryThis PR is a net positive with good architectural decisions. The main concerns are:
Recommendation: Approve with minor changes - address the layer ordering question and add basic testing/verification. Review performed following CLAUDE.md conventions. No formatting changes made per project guidelines. |
MasterPtato
force-pushed
the
10-29-fix_integrate_sentry_with_tracing_and_axum
branch
from
3d0a8c8 to
9ba6fbc
Compare
PR Review: Integrate Sentry with Tracing and AxumI've reviewed this PR that upgrades Sentry integration and consolidates error reporting through the tracing layer. Overall, this is a solid improvement that modernizes the Sentry integration and reduces code duplication. Here are my findings: ✅ Strengths
🔍 Issues & ConcernsCritical: Missing Sentry Dependency in runtime packageLocation: The PR adds Action: Ensure Medium: Layer Ordering in Tracing SubscriberLocation: let registry = registry
.with(reload_layer)
.with(otel_metric_layer)
.with(otel_trace_layer)
.with(sentry::integrations::tracing::layer()) // ← Added here
.with(SentryOtelLayer);Concern: The Question: Does the Sentry layer capture events in Recommendation: Consider reordering to: .with(SentryOtelLayer) // Set trace context first
.with(sentry::integrations::tracing::layer()) // Then let Sentry layer captureOr verify through testing that the current order correctly propagates OTel trace IDs to Sentry. Medium: Scope Mutation Thread SafetyLocation: sentry::configure_scope(|scope| {
scope.set_context(
"trace",
sentry::protocol::TraceContext { /* ... */ },
);
});Concern:
Recommendation: Use sentry::with_scope(|scope| {
scope.set_context("trace", /* ... */);
// Capture within this scope if needed
}, || {});Actually, looking more closely, since this is in Low: Error Removal Without ReplacementLocations:
The manual Verification Needed: Ensure that the tracing layer automatically captures these errors. Based on the code:
But: The old code set explicit tags like Action: Test that error metadata (error codes, workflow IDs, HTTP status codes) appears in Sentry events after this change. 🎯 Recommendations1. Add Integration TestsSince there are no existing tests in runtime or api-builder packages, consider adding at least a smoke test: #[test]
fn test_sentry_otel_layer_trace_context() {
// Verify OTel trace IDs propagate to Sentry
}2. Document the IntegrationAdd a comment in /// Custom tracing layer that synchronizes OpenTelemetry trace IDs with Sentry.
/// This ensures that errors reported to Sentry are correlated with distributed
/// traces, enabling cross-system debugging.
struct SentryOtelLayer;3. Verify Sentry Feature FlagsThe new features added:
Good choices here. 4. Consider Error Level FilteringThe Sentry tracing layer might capture all error-level logs. Consider adding a filter if you only want specific errors: .with(
sentry::integrations::tracing::layer()
.event_filter(|metadata| {
metadata.level() >= &tracing::Level::ERROR
})
)🔐 Security Considerations✅ No issues found. The Sentry DSN is already in the codebase, so no new secrets exposed. ⚡ Performance ConsiderationsMinor: The
This happens on every trace event. Consider:
Suggestion: fn on_event(&self, event: &tracing::Event<'_>, ctx: Context<'_, S>) {
// Early return for non-error events
if \!matches\!(event.metadata().level(), &tracing::Level::ERROR | &tracing::Level::WARN) {
return;
}
// Rest of implementation...
}📋 Test CoverageMissing: No tests were added for the new Recommendation: Add tests or manually verify:
✅ SummaryThis PR is a net positive with good architectural decisions. The main concerns are:
Recommendation: Approve with minor changes - address the layer ordering question and add basic testing/verification. Review performed following CLAUDE.md conventions. No formatting changes made per project guidelines. |
MasterPtato
force-pushed
the
10-29-fix_integrate_sentry_with_tracing_and_axum
branch
from
9ba6fbc to
8db9f85
Compare
PR Review: Sentry Integration with Tracing and AxumSummaryThis PR modernizes the Sentry integration by leveraging Sentry's built-in tracing support instead of manual Positive Changes1. Cleaner Architecture
2. Dependency Updates
3. OpenTelemetry Integration
Issues & Concerns1. Critical: Missing Error Capture
This should work, but verify that the Sentry layer is capturing these errors in your testing environment. 2. Loss of Structured Context // OLD CODE (removed)
sentry::with_scope(|scope| {
scope.set_tag("status", status_code);
scope.set_tag("group", group);
scope.set_tag("code", code);
// ...
}, || sentry::capture_message(...));The new approach relies on tracing's structured fields, but the Sentry tracing layer may not automatically convert all fields to tags. You might lose searchability in Sentry for fields like Recommendation: Consider using tracing::error!(
status=?status_code,
%group,
%code,
%meta,
%internal,
"http server error"
);
// Add this to preserve Sentry tags
sentry::configure_scope(|scope| {
scope.set_tag("error.group", group);
scope.set_tag("error.code", code);
scope.set_tag("http.status", status_code);
});3. Race Condition in In fn on_event(&self, event: &tracing::Event<'_>, ctx: tracing_subscriber::layer::Context<'_, S>) {
if let Some(span) = ctx.event_span(event) {
let extensions = span.extensions();
// ...
sentry::configure_scope(|scope| {
scope.set_context("trace", ...);
});
}
}Problem: Recommendation: Use // Store trace context in span extensions instead of global scope
// Then extract it when the sentry layer processes the eventAlternatively, verify that Sentry's tracing layer already handles this correlation automatically (it might!). 4. Test Coverage ❓
5. Migration Dependency 📦 Security Considerations✅ No security issues identified. The changes maintain the same error handling boundaries. Performance Considerations✅ Improved: Removed manual scope manipulation and message capturing reduces overhead Code QualityStrengths:
Style Compliance:
Recommendations
Questions for Author
VerdictApprove with minor concerns - This is a well-structured modernization of the Sentry integration. The architectural improvements are sound, but please address the structured context preservation and verify end-to-end functionality before merging. The core logic is correct, and this reduces maintenance burden significantly. Just ensure the migration is verified in a real environment. |
NathanFlurry
deleted the
10-29-fix_integrate_sentry_with_tracing_and_axum
branch
No description provided.