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 trace.WithRecord() trace start option #787

Closed
jmacd opened this issue Jun 2, 2020 · 5 comments
Closed

Remove trace.WithRecord() trace start option #787

jmacd opened this issue Jun 2, 2020 · 5 comments
Labels
area:trace Part of OpenTelemetry tracing blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made
Milestone

Comments

@jmacd
Copy link
Contributor

jmacd commented Jun 2, 2020

The spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#isrecording

Some history:
#223
#188
#192

The task is to implement Span.IsRecording() bool which returns true if the span is not a no-op.

@evantorrie
Copy link
Contributor

@jmacd Can you clarify this a little? I thought #188 did change/add the IsRecording() bool function to the Span interface and all implementations.

@jmacd
Copy link
Contributor Author

jmacd commented Jun 8, 2020

I realize that IsRecording() is implemented. I thought it had been removed.

@jmacd jmacd closed this as completed Jun 8, 2020
@jmacd jmacd reopened this Jun 8, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Jun 8, 2020

Ah, the spec does not support a WithRecord() function that we currently have. It's the sampler's responsibility for deciding if a span records.

@jmacd jmacd changed the title Implement IsRecording Remove trace.WithRecord() trace start option Jun 8, 2020
@evantorrie
Copy link
Contributor

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 added blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made area:trace Part of OpenTelemetry tracing labels Jul 2, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Sep 25, 2020

I think this has become a duplicate of #192, going to close this and consolidate the conversation there.

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 blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made
Projects
None yet
Development

No branches or pull requests

4 participants