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

Remove WithRecording option from trace API #192

Closed
iredelmeier opened this issue Oct 11, 2019 · 3 comments · Fixed by #1660
Closed

Remove WithRecording option from trace API #192

iredelmeier opened this issue Oct 11, 2019 · 3 comments · Fixed by #1660
Assignees
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Milestone

Comments

@iredelmeier
Copy link
Member

iredelmeier commented Oct 11, 2019

This option needs to be removed before the GA release. The specification decision on how to resolve this issue has been postponed until after GA. To reduce the maintenance burden of this code after GA and to ensure we can support the future resolution of this issue at the specification level we need to remove this option.

Work needs to be done to explore how this option can be removed and still conform to the SDK specification sampling.

@danstephen88
Copy link

Say again

@paivagustavo
Copy link
Member

When we tried to remove this (#223), we found out about the problem discussed in #224. Blocking this until open-telemetry/opentelemetry-specification#307 to be resolved.

@paivagustavo paivagustavo added the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Dec 6, 2019
@MrAlias MrAlias removed the good first issue Good for newcomers label Apr 29, 2020
@MrAlias MrAlias added this to the RC1 milestone Aug 27, 2020
@MrAlias MrAlias added priority:p1 and removed blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made priority:p3 labels Sep 25, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Sep 25, 2020

Info from duplicate issue #787

Was there ever a resolution to open-telemetry/opentelemetry-specification#307

I see @paivagustavo tried in #223 to do this, and ran into the same problem that span.SetName() requires a resampling decision. In the Go implementation, if the span did not already have IsRecording() == true, then there's no data for it to make that decision.

It doesn't look like there has been any change made to the specification and thus calling SetName() still seems to require another call to the sampler.

Found some more issues related to this for background reading:
open-telemetry/opentelemetry-specification#468
#224

and more recently

open-telemetry/opentelemetry-specification#620
open-telemetry/opentelemetry-specification#634

@MrAlias MrAlias changed the title Remove WithRecordingEvents option Remove WithRecording option from trace API Sep 25, 2020
@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package labels Sep 25, 2020
hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
…aws (#192)

* Bump github.com/aws/aws-sdk-go from 1.33.16 to 1.33.19 in /detectors/aws

Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.33.16 to 1.33.19.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Changelog](https://github.com/aws/aws-sdk-go/blob/master/CHANGELOG.md)
- [Commits](aws/aws-sdk-go@v1.33.16...v1.33.19)

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
evantorrie added a commit to evantorrie/opentelemetry-go that referenced this issue Mar 5, 2021
MrAlias added a commit that referenced this issue Mar 9, 2021
…#1660)

* Remove `WithRecord()` option from SpanConfig options

This brings the trace API into conformance with the specification.

* Add entry to CHANGELOG

Fixes #192

* Updated CHANGELOG with PR#

* Cleaned up CHANGELOG notes

* fixup! Merge remote-tracking branch 'upstream/main' into remove-with-record

* Use new spanContext API to set traceflags, tracestate

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
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
None yet
6 participants