slashmo / gsoc-swift-tracing

WIP collection of Swift libraries enabling Tracing on Swift (Server) systems.
https://summerofcode.withgoogle.com/projects/#6092707967008768
Apache License 2.0
21 stars 1 forks source link

Remove timestamp, kind and status getters from Span #127

Closed pokryfka closed 4 years ago

pokryfka commented 4 years ago

Motivation:

Follow OTel recommendations:

After the Span is created, it SHOULD be possible to change the its name, set its Attributes, and add Links and Events.

Spans are not meant to be used to propagate information within a process. To prevent misuse, implementations SHOULD NOT provide access to a Span's attributes besides its SpanContext.

which prevent from abuse.

On top of that it make it simpler to implement (conform to Span), for example if a Span implementation does not have direct SpanStatus equivalent (X-Ray segment):

slashmo commented 4 years ago

Do you also want to include updating the name in this PR as it's contained in the OTel quote you added?

After the Span is created, it SHOULD be possible to change the its name, set its Attributes, and add Links and Events.

pokryfka commented 4 years ago

could you please also check that the UseCases package still compiles? I believe it should currently fail as you changed the requirements of the Span protocol.

will test it, sorry if I did

I assumed it would be caught in CI tests (?)


Do you also want to include updating the name in this PR as it's contained in the OTel quote you added?

After the Span is created, it SHOULD be possible to change the its name, set its Attributes, and add Links and Events.

its just a quote from OTel, personally I dont see a reason to let user update name but I will comply if you think it makes sense (deos it?) or want to follow OTel as closely as possible;

note that OTel do NOT recommend to implement "name" getter (I made it public just to satisfy Span.operationName, I think its okey to keep it that way, even though user should NOT really ever need it; or he does need to propagate the name, the span should not be the way to do so)

@ktoso WDYT?

ktoso commented 4 years ago

note that OTel do NOT recommend to implement "name" getter (I made it public just to satisfy Span.operationName, I think its okey to keep it that way, even though user should NOT really ever need it;

Yeah sounds good to me to remove as much getters as possible from the span -- removing the name sounds good. People should not be "inspecting" those at runtime.

ktoso commented 4 years ago

Maybe the span name changing is for situations where you want to start a span, but you for some reason will later have a "better name for it" (no idea why...), so I can see why setting again may be ok... Let's adhere to the OTel suggestion just to make it easier for people who are following that to port their tracers.

Sorry for the crappy impl of the span in the traced lock, I'm fairly sure it is very wrongly implemented, it was just to PoC the instrumenting a lock with some printouts; pretty sure it's wrong in the impl of requirements.

pokryfka commented 4 years ago

Removed operationName getter from Span protocol.

As for updateName I recalled that some SDKs (also AWS XRay tracers - APIs differ quite a lot depending on language) "generate" segments using function builders or decorators.

In such scenario span would not be created explicitly by user, but its nice to let him change generated the name to be more meaningful.

I do not think its applicable to Swift as of now.

ktoso commented 4 years ago

As for updateName I recalled that some SDKs (also AWS XRay tracers - APIs differ quite a lot depending on language) "generate" segments using function builders or decorators.

Yeah that's a good use case... true that we dont have those yet.

LGTM; good to go @slashmo ?

ktoso commented 4 years ago

resolves https://github.com/slashmo/gsoc-swift-tracing/issues/87