refactor(sentry): replace manual flush with context lifecycle management#1070
Conversation
- Remove SentrySdkAspect (AOP interception for SentrySdk init/hub/context) - Use SentrySdk::startContext()/endContext() in CoroutineAspect - Remove all manual SentrySdk::flush() calls from Metrics listeners (OnBeforeHandle, OnCoroutineServerStart, OnMetricFactoryReady, OnWorkerStart, PoolWatcher, QueueWatcher) - Remove SentrySdk::flush() from Tracing CoroutineAspect and EventHandleListener - Unregister SentrySdkAspect from ConfigProvider
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ext" This reverts commit fd289ca.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ddd4bf6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if ($this->globalContext === null) { | ||
| // Lazy fallback keeps baseline behavior when users do not opt into explicit context lifecycle. | ||
| $this->globalContext = new RuntimeContext('global', $this->baseHub); |
There was a problem hiding this comment.
When a request/RPC/command is handled without going through Hyperf\Coroutine\Coroutine::create, it now falls back to this single global RuntimeContext, so Tracing\Listener\EventHandleListener::handleRequestReceived() sets spans/scopes on a hub shared by concurrent requests. The removed SentrySdkAspect previously kept the runtime manager in Hyperf's coroutine Context, but the only new startContext() calls are in the coroutine aspects, leaving normal server lifecycle events unisolated and allowing breadcrumbs/spans/metrics to bleed across requests until some later flush.
Useful? React with 👍 / 👎.
…ent (#1070) * refactor(sentry): replace manual flush with context lifecycle management - Remove SentrySdkAspect (AOP interception for SentrySdk init/hub/context) - Use SentrySdk::startContext()/endContext() in CoroutineAspect - Remove all manual SentrySdk::flush() calls from Metrics listeners (OnBeforeHandle, OnCoroutineServerStart, OnMetricFactoryReady, OnWorkerStart, PoolWatcher, QueueWatcher) - Remove SentrySdk::flush() from Tracing CoroutineAspect and EventHandleListener - Unregister SentrySdkAspect from ConfigProvider * chore(sentry): bump sentry/sentry minimum version to ^4.28.0 * chore(sentry): sync sentry/sentry version in root composer.json * ♻️ refactor(sentry): remove unused SingletonAspect and clean up comments * 🐛 fix(sentry): move startContext into coroutine after context restore * ✨ feat(sentry): add coroutine-safe RuntimeContextManager via class_map * 📝 docs(sentry): add comment clarifying ContextArrayObject usage in constructor * 🐛 fix(sentry): ensure baseHub has a valid client in RuntimeContextManager * ♻️ refactor(sentry): move context lifecycle into tracing aspect * 📝 docs(sentry): correct type hints and comment for ContextArrayObject * 🐛 fix(sentry): run metrics collection within a coroutine context * 🐛 fix(sentry): disable coordinator tracing span by default * ♻️ refactor(sentry): remove coordinator tracing aspect * Revert "🐛 fix(sentry): run metrics collection within a coroutine context" This reverts commit fd289ca. * 🐛 fix(sentry): isolate sentry context per coroutine * 🐛 fix(sentry): treat sentry metrics/transport as coroutine backtrace break points * 📦 build(sentry): require hyperf/coordinator --------- Co-Authored-By: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
1 participant
Summary
Refactor Sentry integration to use
SentrySdk::startContext()/endContext()for coroutine context management, replacing the previous pattern of manualSentrySdk::flush()calls scattered across the codebase.Background
The previous implementation manually called
SentrySdk::flush()in multiple listeners and aspects after each operation. Additionally,SentrySdkAspectinterceptedSentrySdkmethods via AOP to manage coroutine-safe hub/context state. This approach was fragile and scattered flush logic across many files.Changes
SentrySdkAspect— Remove AOP interception ofSentrySdk::init,setCurrentHub,getRuntimeContextManagerCoroutineAspectrefactor — UseSentrySdk::startContext()before coroutine starts andSentrySdk::endContext()on defer, replacing the oldSentrySdk::flush()patternSentrySdk::flush()calls from:Metrics/Listener/OnBeforeHandleMetrics/Listener/OnCoroutineServerStartMetrics/Listener/OnMetricFactoryReadyMetrics/Listener/OnWorkerStartMetrics/Listener/PoolWatcherMetrics/Listener/QueueWatcherTracing/Aspect/CoroutineAspectTracing/Listener/EventHandleListenerSentrySdkAspectfromConfigProviderTest Plan
Risks
startContext()/endContext()APIs must be available in the target Sentry SDK version. Verify compatibility before merging.