api(trace): remove 'WithRecord' span option. #223
Conversation
paivagustavo
requested review from
bogdandrutu,
jmacd,
pjanotti,
rghetia,
sjkaris and
tedsuo
as code owners
As per the spec it should be renamed. There was an issue in spec about changing it to SetName, however it was closed with the intention to reopen the discussion in otep. There is no otep created AFAIK.
SetName (updateName) api was introduced to set the name at later time. Given that sampling decision depends on the name, it is important to record events and attributes just in case if the span is sampled at a later time when the name is set. WithRecord option allowed to do that. Now, if WithRecord is removed, how do we achieve delayed sampling? |
|
We need to keep two independent bits: I think the confusion over sampling APIs as it relates to the |
|
One possibility is that the Sampler returns RECORD decision at the time of span creation and later when it is called again, it returns same or different decision. |
|
@rghetia Yeah, that sort of solution would address my concern about the sampling API. Then we can leave in |
|
closed the PR by mistake. |
| // TODO (paivagustavo): this test used `WithRecord` to force the span to have | ||
| // a export data, this ensured that `SetName` worked nicely, but `WithRecord` | ||
| // was removed from the API. | ||
| //{ // 2 |
There was a problem hiding this comment.
Lets' open another issue to track this. One possible solution is listed here but it requires Sampling parameter to have some indication that the sampler is being called again.
There was a problem hiding this comment.
Ok, I'll create another issue to track this. I think we need to discuss a little further about the problem, I'll try to explain the consequences of doing this.
There was a problem hiding this comment.
This is OK on the surface--we'll remove the WithRecord span option. But the trouble it exposes with UpdateName vs SetName is real, and I think we should fix the spec so that there is one API to control whether a span may be sampled (i.e., is recorded) and one API to decide whether to sample at the end.
|
I feel that we need to keep the functionality of having a span be recorded but not sampled, which means finding a spec-approved way to set it. This is being discussed in open-telemetry/opentelemetry-specification#307 |
|
postpone until spec has resolution. |
|
Closing this to not pollute the repo until we have a specs resolution. |
This remove the
WithRecordspan option, Closes #192.Removing this option made clear that we have a problem with the
SetName(should this be renamed toUpdateName?) method. Currently, when a span is not sampled, we don't create a complete span, i.e., there is noSpanData. Because of that, we lose the span's start time, if it came from a remote parent and all the initial SpanOptions.We probably need to take this to another issue. From what I could understand of the Java client, they have the exact same problem, maybe this should go to specs since it appears that samplers can drop these by specs.