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

Fix span isRecording #67

Closed
wants to merge 14 commits into from
Closed

Fix span isRecording #67

wants to merge 14 commits into from

Conversation

bryan-aguilar
Copy link

Fixes open-telemetry#1664

Also fixes a test case bug that arose after the isRecording() bug was fixed.

@bryan-aguilar bryan-aguilar marked this pull request as draft March 26, 2021 22:58
@bryan-aguilar bryan-aguilar marked this pull request as ready for review March 26, 2021 23:10
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
bryan-aguilar and others added 2 commits March 29, 2021 09:59
Improve readability of test name

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Improve readability of test name

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Copy link

@alolita alolita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For final PR pl fix title and make the description more readable :-)

@bryan-aguilar bryan-aguilar changed the title Is recording issue Fix span isRecording Mar 29, 2021
bryan-aguilar and others added 6 commits March 29, 2021 13:20
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Update SpanSnapshot to use parent SpanContext

Having only the parent span ID and a separate field to communicate if
the parent was remote does not provide a comprehensive view of the
parent span nor is it an efficient way to transmit this information.
Update the SpanSnapshot to have a `Parent` field that contains the
parent span context. This field replaces the ParentSpanID and
HasRemoteParent fields.

* Revert SamplingParameters span change

* Update CHANGELOG with PR number
* Update SamplingParameters

Remove HasRemoteParent fields from SamplingParameters. The
HasRemoteParent field is a duplicate of the Remote field of the parent
span context contained in the ParentContext.

Change the `ParentContext` field from storing a `SpanContext` to a
`context.Context` that holds the parent span. This is to conform with
the OpenTelemetry specification and resolve open-telemetry#1727.

* Update PR number
@codecov-io
Copy link

Codecov Report

Merging #67 (156686a) into main (604b05c) will increase coverage by 38.2%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##            main     #67      +/-   ##
========================================
+ Coverage   39.8%   78.1%   +38.2%     
========================================
  Files        179     132      -47     
  Lines      13942    6948    -6994     
========================================
- Hits        5562    5430     -132     
+ Misses      8023    1272    -6751     
+ Partials     357     246     -111     
Impacted Files Coverage Δ
exporters/otlp/internal/otlptest/data.go 64.0% <100.0%> (+2.2%) ⬆️
exporters/otlp/internal/transform/span.go 98.1% <100.0%> (ø)
exporters/trace/jaeger/jaeger.go 93.5% <100.0%> (+<0.1%) ⬆️
exporters/trace/zipkin/model.go 98.6% <100.0%> (ø)
sdk/trace/sampling.go 93.9% <100.0%> (+0.1%) ⬆️
sdk/trace/span.go 94.0% <100.0%> (+3.5%) ⬆️
trace/context.go 100.0% <100.0%> (ø)
.../third_party/thrift/lib/go/thrift/configuration.go
...third_party/thrift/lib/go/thrift/zlib_transport.go
...hird_party/thrift/lib/go/thrift/binary_protocol.go
... and 45 more

@Aneurysm9 Aneurysm9 closed this Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants