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

API: Span - use SetName instead of UpdateName #119

Closed
mayurkale22 opened this issue Jun 14, 2019 · 13 comments
Closed

API: Span - use SetName instead of UpdateName #119

mayurkale22 opened this issue Jun 14, 2019 · 13 comments
Assignees
Labels
area:api Cross language API specification issue needs discussion Need more information before all suitable labels can be applied
Milestone

Comments

@mayurkale22
Copy link
Member

@SergeyKanzhelev
Copy link
Member

we had a few discussions regarding this name. some reflections of those discussions are here: open-telemetry/opentelemetry-java#23

Basically the issue with setName is that we need to emphasize that this is a major change for a Span and is it not the same as set attribute.

In node I'd suggest to put a big remark in code that the name is not aligned with the spec. Ideally - change it to updateName before this issue is approved. So terminology will be aligned across languages.

@SergeyKanzhelev SergeyKanzhelev added the area:api Cross language API specification issue label Jun 14, 2019
@SergeyKanzhelev SergeyKanzhelev added this to the API revision: 07-2019 milestone Jun 14, 2019
@rochdev
Copy link
Member

rochdev commented Jun 17, 2019

I think this depends mostly on semantics. Is it possible to create span with no name, and only set the name later on? If it's not possible, then I agree updateName better describes the behavior.

@SergeyKanzhelev
Copy link
Member

name is the only parameter we ask for when creating a SpanBuilder. So it is absolutely required.

@jmacd
Copy link
Contributor

jmacd commented Jun 17, 2019

I feel that SetName is more consistent with other methods on Span.

@SergeyKanzhelev
Copy link
Member

The name highlights the fact that it is doing a different thing and is more dangerous than simple Set method. For instance, it wouldn't re-apply sampling. Thus it was made non-consistent with other Set names.

@jmacd
Copy link
Contributor

jmacd commented Jun 17, 2019

I don't see how the API and sampling are related. Sampling is an SDK function, right?

@SergeyKanzhelev
Copy link
Member

The name highlights the fact that it is doing a different thing and is more dangerous than simple Set method. For instance, it wouldn't re-apply sampling. Thus it was made non-consistent with other Set names.

@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

I've begun to question why the Sampler is considered part of the API (#164). I see this "more dangerous" statement you made above as an implementation detail. The OpenTracing StartSpanOptions allow specifying initial attributes when starting a span. If the sampler were to use those attributes to make its decision, then should we conceivably rename SetAttribute to UpdateAttribute? I think not.

I do think it's important to allow setting a name, for cases where a Span object has to be started before parsing the name. I just don't see it as important to use a special verb to update that name.

@rochdev
Copy link
Member

rochdev commented Jul 6, 2019

I tend to agree with @jmacd that it should be consistent. If it's a setter it should have a setter name. This is especially true for languages that could replace setters with actual properties.

So I would propose one of these 2 possibilities:

  • If it's considered so dangerous, just remove it completely.
  • Otherwise rename to setName as is standard and consistent.

@z1c0
Copy link
Contributor

z1c0 commented Jul 9, 2019

Having the possibility to update a span's name later is definitely a valid use case.
Also, adding a way to "freeze" or "finalize" a span so that subsequent calls to UpdateName will have no effect or fail could be useful.

@iredelmeier iredelmeier added the needs discussion Need more information before all suitable labels can be applied label Jul 30, 2019
@SergeyKanzhelev SergeyKanzhelev removed this from the API revision: 07-2019 milestone Sep 27, 2019
@bogdandrutu bogdandrutu added this to the Alpha v0.2 milestone Sep 30, 2019
@SergeyKanzhelev SergeyKanzhelev self-assigned this Oct 3, 2019
@SergeyKanzhelev
Copy link
Member

I'm adding a PR that adds explanation for the current name on why it is different from setter.

@bogdandrutu
Copy link
Member

+1 @SergeyKanzhelev. I think it is important to use "Update" to reflect that in system that do advance sampling this is different than setting a name.

@SergeyKanzhelev
Copy link
Member

@mayurkale22 @jmacd and @OlivierAlbertini I understand you believe the name change is needed. I'm closing this issue, please consider filing an OTEP for the name change with the description on why it is needed.

As I said this issue was discussed during API (v0.1) work group meetings. And this name reflects interests of vendors with the high-load apps where the head-based sampling and the early name setting is very critical.

So the name change to the setter semantic suggestion is more than just a formality.

Please re-open this issue if you do not agree that this issue requires an OTEP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue needs discussion Need more information before all suitable labels can be applied
Projects
None yet
Development

No branches or pull requests

7 participants