Skip to content
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

Redefine ExportSpans of SpanExporter with ReadOnlySpan #1873

Merged
merged 18 commits into from May 4, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented May 2, 2021

This updates the SpanExporter interface to accept ReadOnlySpans instead of SpanSnapshots. This includes the following changes:

  • The SpanStub type and its associated functions were added to the go.opentelemetry.io/otel/sdk/trace/tracetest package. This type can be used as a testing replacement for the SpanSnapshot that was removed from the go.opentelemetry.io/otel/sdk/trace package.
  • The ExportSpans method of theSpanExporter interface type was updated to accept ReadOnlySpans instead of the removed SpanSnapshot. This brings the export interface into compliance with the specification in that it now accepts an explicitly immutable type instead of just an implied one.
  • Removed the Tracer and IsRecording method from the ReadOnlySpan in the go.opentelemetry.io/otel/sdk/trace. The Tracer method is not a required to be included in this interface and given the mutable nature of the tracer that is associated with a span, this method is not appropriate. The IsRecording method returns if the span is recording or not. A read-only span value does not need to know if updates to it will be recorded or not. By definition, it cannot be updated so there is no point in communicating if an update is recorded.
  • Removed the SpanSnapshot type from the go.opentelemetry.io/otel/sdk/trace package. The use of this type has been replaced with the use of the explicitly immutable ReadOnlySpan type. When a concrete representation of a read-only span is needed for testing, the newly added SpanStub in the go.opentelemetry.io/otel/sdk/trace/tracetest package should be used.
  • Rename all *messageEvent* names to just be *Event*.

Resolves #1380

This is not required by the specification nor the use of this interface.
Use this type to encapsulate the Span status similar to the Event type
encapsulating a Span event
A read-only span value does not need to know if updates to it will be
recorded. It by definition cannot be updated so no point in
communicating if an update would be recorded.
Add the DroppedAttributes, DroppedLinks, DroppedEvents, and
ChildSpanCount methods to the interface to return additional information
about the span not specified by the specification, but that we are
already providing.
This method is a hold-over from previous version of the ReadOnlySpan
interface is not needed.
@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #1873 (187e4c0) into main (c99d5e9) will decrease coverage by 0.0%.
The diff coverage is 89.2%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1873     +/-   ##
=======================================
- Coverage   79.0%   79.0%   -0.1%     
=======================================
  Files        137     139      +2     
  Lines       7384    7477     +93     
=======================================
+ Hits        5837    5909     +72     
- Misses      1301    1322     +21     
  Partials     246     246             
Impacted Files Coverage Δ
exporters/otlp/protocoldriver.go 100.0% <ø> (ø)
sdk/trace/simple_span_processor.go 88.0% <0.0%> (-0.5%) ⬇️
exporters/stdout/trace.go 69.5% <50.0%> (+4.3%) ⬆️
sdk/trace/snapshot.go 52.9% <52.9%> (ø)
sdk/trace/span.go 88.9% <71.0%> (-5.0%) ⬇️
sdk/trace/batch_span_processor.go 87.1% <75.0%> (ø)
sdk/trace/tracetest/span.go 96.7% <96.7%> (ø)
exporters/otlp/internal/otlptest/data.go 88.0% <100.0%> (+0.2%) ⬆️
exporters/otlp/internal/transform/span.go 98.1% <100.0%> (ø)
exporters/otlp/otlp.go 83.9% <100.0%> (ø)
... and 9 more

@MrAlias MrAlias mentioned this pull request May 2, 2021
@jmacd
Copy link
Contributor

jmacd commented May 3, 2021

I really read it. :)

@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package labels May 3, 2021
@MrAlias MrAlias marked this pull request as ready for review May 3, 2021 19:37
@MrAlias MrAlias added this to In progress in OpenTelemetry Go RC via automation May 3, 2021
@MrAlias MrAlias added this to the RC1 milestone May 3, 2021
OpenTelemetry Go RC automation moved this from In progress to Reviewer approved May 4, 2021
@MrAlias MrAlias merged commit cbcd4b1 into open-telemetry:main May 4, 2021
OpenTelemetry Go RC automation moved this from Reviewer approved to Done May 4, 2021
@MrAlias MrAlias deleted the ro-span-export branch May 4, 2021 23:45
@Aneurysm9 Aneurysm9 mentioned this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Clarify package demarcation between sdk/trace and sdk/export/trace
4 participants