-
-
Notifications
You must be signed in to change notification settings - Fork 277
[Span First #1]: Add simple span API #3360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/span-first #3360 +/- ##
===================================================
- Coverage 88.31% 87.91% -0.41%
===================================================
Files 291 294 +3
Lines 9957 9991 +34
===================================================
- Hits 8794 8784 -10
- Misses 1163 1207 +44 ☔ View full report in Codecov by Sentry. |
| import 'span.dart'; | ||
| import 'span_v2_status.dart'; | ||
|
|
||
| class SimpleSpan implements Span { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also open to calling this SpanImpl or something different. The gist is that this is the default span impl
|
As discussed we will change this to using an internal |
| } | ||
|
|
||
| return NoOpSpan(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Redundant conditional logic, both branches return same value
The startSpan method contains redundant conditional logic where all code paths return NoOpSpan(), making the if (!_isEnabled) and else if (_options.isTracingEnabled()) checks meaningless. When tracing is enabled, the method should perform actual implementation logic (as indicated by the TODO), but currently just returns the same no-op as disabled paths. Either the conditional branches are unnecessary, or the enabled branch should contain actual implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just scaffolding, real impl will follow in the next PR
📜 Description
Span First initiative
💡 Motivation and Context
Part of #3331
This is only basic API scaffolding
💚 How did you test it?
No tests yet since there is no specific logic yet
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps
#skip-changelog