Fix charge span leak in the payment service#3276
Fix charge span leak in the payment service#3276ayoisaiah wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
osullivandonal
left a comment
There was a problem hiding this comment.
Nice work on this, thanks for the contribution! Could we add a changelog entry to CHANGELOG.md.
There was a problem hiding this comment.
Pull request overview
Fixes OpenTelemetry span lifecycle and error attribution in the payment service so the manually-created charge span is always ended and owns its own exceptions (instead of leaking or attributing exceptions to the parent gRPC span), addressing issue #3275.
Changes:
- Wrap
charge()logic intry/catch/finallyto guaranteespan.end()on all paths and to record exception + ERROR status on thechargespan. - Remove explicit
recordException()on the parent gRPC span and set its status to ERROR with the error message to avoid duplicate exception attribution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/payment/index.js | Stops recording exceptions on the parent gRPC span; sets ERROR status with message only. |
| src/payment/charge.js | Ensures charge span is always ended and that exceptions/status are recorded on the charge span. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes
Fix span lifecycle and exception attribution in the payment service charge span.
charge()in atry/catch/finallysospan.end()is always called, including on validation error paths where the span previously leaked.SpanStatusCode.ERRORon the charge span in the catch block, where the failure actually originates.span.recordException(err)fromindex.js. The gRPC span now only has its status set toERRORwith the error message, avoiding duplicate exception attribution.Fixes #3275
Merge Requirements
For new features contributions, please make sure you have completed the following
essential items:
CHANGELOG.mdupdated to document new feature additions