feat: Set segment name on scope forNoOpStreamedSpan - #7162
feat: Set segment name on scope forNoOpStreamedSpan#7162alexander-alderman-webb wants to merge 9 commits into
NoOpStreamedSpan#7162Conversation
NoOpStreamedSpanNoOpStreamedSpan
Codecov Results 📊✅ 104113 passed | ⏭️ 6677 skipped | Total: 110790 | Pass Rate: 93.97% | Execution Time: 362m 27s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2485 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 90.15% 90.12% -0.03%
==========================================
Files 193 193 —
Lines 25147 25153 +6
Branches 9136 9140 +4
==========================================
+ Hits 22669 22668 -1
- Misses 2478 2485 +7
- Partials 1431 1431 —Generated by Codecov Action |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 63e764a. Configure here.
sentrivana
left a comment
There was a problem hiding this comment.
Looks good, just one question.
|
|
||
| class NoOpStreamedSpan(StreamedSpan): | ||
| __slots__ = ( | ||
| "_noop_name", |
There was a problem hiding this comment.
I assume we can't use the existing _name slot for this?
There was a problem hiding this comment.
The parent _name is typed as str, and all the __init__ parameters are optional for NoOpStreamedSpan.
There was a problem hiding this comment.
Don't we do something similar for scope though? In the parent class it's not optional, but in NoOpStreamedSpan it is, and it uses the same slot.
I guess that's what the # type: ignore[assignment] is there for. TBH I'd prefer we do the same with _name to keep stuff consistent.
There was a problem hiding this comment.
I don't mind, fair to follow the existing pattern: f4986d9
Description
Add the
nameparameter toNoOpStreamedSpan.__init__().Add a
_noop_namefield toNoOpStreamedSpan(unlike_namein the superclass, it's optional).Set the transaction name on the scope when a segment
NoOpStreamedSpanis set on the scope.The commit 11c1e90 removed validation of tracing without performance, so I've added a new test where tracing is disabled.
Issues
Reminders
uv run ruff.feat:,fix:,ref:,meta:)