Span Refactor#422
Conversation
Drop support for PHP 7.3
Update Exporters to use `SpanData` Start updating Exporter tests Implement non immutable `SpanData` test helper class
Leverage a `SpanBuilder` to create the span Use a dedicated object to share state
Start of adding tests for new Tracer implementation
| use OpenTelemetry\Sdk\Trace\StatusData; | ||
| use OpenTelemetry\Trace as API; | ||
|
|
||
| class SpanData implements SDK\SpanData |
There was a problem hiding this comment.
This is a mutable version of SpanData, mainly useful in tests as a mini builder so you don't have to provide every span constructor argument.
There was a problem hiding this comment.
Should probably do some composer something or other so this doesn't get shipped when installing the package. I.e. should be a development type only.
There was a problem hiding this comment.
You might need to move it into its own separate file and ignore it if that's what you'd like to do. I'm sure there is a way to do it for a particular class but I don't know that answer off the top of my head.
bobstrecansky
left a comment
bobstrecansky
left a comment
There was a problem hiding this comment.
A couple initial comments but I don't think I have any comments that are blockers currently.
Make `Sampler` the 2nd arg to `TraceProvider` Fix Integration tests
Continue updating/adding tests
Ensure span processor is called with expected contexts/spans
| API\Events $events, | ||
| API\Attributes $attributes, | ||
| int $totalAttributeCount, | ||
| int $totalRecordedEvents, |
There was a problem hiding this comment.
Is it possible to have count($events) !== $totalRecordedEvents?
A Span should report a number of Events dropped: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#additional-span-interfaces
There was a problem hiding this comment.
Is it possible to have
count($events) !== $totalRecordedEvents?
Yea, in the case more events were added than the allowed limit. $totalRecordedEvents would be incremented but they wouldn't be actually added to the list of events. Using this method it's possible to calculate the amount of dropped events
But thanks for that link, maybe we could change the interface a bit to expose the dropped count based on total - events.size instead of just the total amount. That would probably be a bit more spec compliant.
Make `SpanProcessor#onStart` take a `ReadWriteSpan` Update empty span fallback name
Continued work on updating test coverage
Slightly updates the implementation to get things working with new `Span` implementation
Codecov Report
@@ Coverage Diff @@
## main #422 +/- ##
============================================
- Coverage 94.33% 91.30% -3.03%
- Complexity 758 799 +41
============================================
Files 63 67 +4
Lines 1854 1979 +125
============================================
+ Hits 1749 1807 +58
- Misses 105 172 +67
Continue to review full report at Codecov.
|
Add test coverage for these cases
3 participants
Still some work to be done to ensure everything is in an actual working state, but wanted to get this created to start a discussion around the approaches and such.
Sorry for the large PR, but it's kinda core and touches a lot of stuff :/
What this PR does:
Spanconstructor and how you create a spanSpanDatainterface that represents an immutable snapshot of a span mainly for use as part of span exportersTracerimplementation to leverage aSpanBuilderversus relying on multiple methods with variable amount of argumentsNoopSpantoNonRecordingSpanand makes it only implementSpan, notReadWriteSpan. Fixes Implement no-op span behavior per spec #266TracerProviderandTracernow leverage aTracerSharedStatein order to share the instances of idGenerator, sampler, etc.Clockinterface, rename SDKClocktoSystemClockand introduceTestClockClockinstance to use by default, e.g. for testingSpanContextconstructor methods into 2, one for remote and one for non remote contextsWhat still needs to be done:
todos where they were needed so shouldn't be too bad