Timestamp to be non optional in TracingInstrument

[ktoso]

Today we offer:

self.startSpan(named: <#T##String##Swift.String#>, context: <#T##BaggageContextCarrier##Baggage.BaggageContextCarrier#>, ofKind: <#T##SpanKind##TracingInstrumentation.SpanKind#>, at: <#T##Timestamp?##TracingInstrumentation.Timestamp?#>), and span.end(at: <#T##Timestamp##TracingInstrumentation.Timestamp#>).

Note the optional Timestamp in start span, it really should be not optional I guess.

Start and end time as well as Event's timestamps MUST be recorded at a time of a calling of corresponding API.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-lifetime

---

Yes, there is a point to be made about "my impl can't take that timestamp because it has some internal way to get the time" (os_signpost) but in general such impls have to ignore the passed in time anyway 🤔

[slashmo]

Yes, that's a good point. I also found a bit strange to pass in nil to basically tell the TracingInstrument to use the current timestamp. How about making it a required param, defaulting to .now() in an extension?

[ktoso]

How about making it a required param, defaulting to .now() in an extension?

Yeah, let's go with that.

[pokryfka]

TL;DR 100% agree this makes the API nicer but I think its also much less practical, or at least less pragmatical, please see details below.

Start timestamp, default to current time. This argument SHOULD only be set when span creation time has already passed. If API is called at a moment of a Span logical start, API user MUST not explicitly set this argument.

Assuming:

• 99.999% of the time segments/spans should start and end "now"
• every instrument/tracer will have its own implementation/type to represent time
• every type representing timestamp will have an optimized convenience initializer to get now value

Its not practical to provide TracingInstrument.Timestamp now value.

What it means is that TracingInstrument implementation needs to parse the value of Timestamp now
then map to its own type.

Optional timestamp value (when starting end ending span) allowed TracingInstrument implementation to use his own "now" without type mapping, this is not possible anymore.

So, just an idea, perhaps we could pass the concept of "now" without any value, sth like:

extension Span {
  public enum Timestamp {
    case now
    case unixTimestamp(milisecs: UInt64)
  }
}

Note that this would also allow to easily extend type/precision if needed and let TracingInstrument decide how to implement it.

[ktoso]

(I'd prefer if you opened up new tickets rather than commenting on closed ones please @pokryfka, it makes tracking changes harder -- reopening this for discussion).

[pokryfka]

I'd prefer if you opened up new tickets rather than commenting on closed ones

ok, will do next time

another idea is to have startSpan functions with and without the timestamp type in the protocol TracingInstrument

public protocol TracingInstrument: Instrument {

    func startSpan(
        named operationName: String,
        context: BaggageContextCarrier,
        ofKind kind: SpanKind,
        at timestamp: Timestamp
    ) -> Span

    func startSpan(
        named operationName: String,
        context: BaggageContextCarrier,
        ofKind kind: SpanKind,
    ) -> Span

}

[ktoso]

I'm not sure I buy the specific arguments used here @pokryfka:

99.999% of the time segments/spans should start and end "now"

Yes, that's very true. Perhaps let's step back and remove the param altogether, see proposal below the ---.

every instrument/tracer will have its own implementation/type to represent time

Why have to? The entire purpose is for the type to be the timestamp.

I don't see your or other implementations be much different. But again, I think we should rather either completely remove the parameter or embrace it. Completely removing makes sense because eventually we want the swift stdlib to offer such time types anyway.

every type representing timestamp will have an optimized convenience initializer to get now value
Its not practical to provide TracingInstrument.Timestamp now value.

What it means is that TracingInstrument implementation needs to parse the value of Timestamp now
then map to its own type.

Not practical, because...? Why "parse", it isn't a parse, it is accepting a provided value.

---

In any case though: Let's propose an alternative take on this...

Proposal: How about we just drop the timestamp parameter all-together. If an implementation wants to allow mocking time it'd be up to a tracer's implementation, not to the API after all.

    func startSpan(
        named operationName: String, 
          // btw. I'd like to revisit the "named" not in love with it the more code I write using it
          //    e.g. _ operationName
          // what do others think?
        context: BaggageContextCarrier,
        ofKind kind: SpanKind // this is defaulted to .internal
    ) -> Span

Adding two overloads is not good IMO, we should not make it harder to adopt this API by making it confusing which/how function to implement and if one is or isnt expressed in terms of the other etc.

I assume that'd make the xray and other implementations happy as well -- you can create the timestamp however you want, and it is up to a specific tracer if it supports microseconds or nanoseconds. Users lose control of passing in the time explicitly, but realistically, that indeed is a rare use case.

[pokryfka]

every instrument/tracer will have its own implementation/type to represent time

Why have to? The entire purpose is for the type to be the timestamp.

I think it depends if instrument implementation depends directly on swift-tracing and uses its types internally,
or add conformance to TracingInstrument.

I don't see your or other implementations be much different. But again, I think we should rather either completely remove the parameter or embrace it. Completely removing makes sense because eventually we want the swift stdlib to offer such time types anyway.

My implementation is practically the same but it uses its own type, so in every startSpan if the value of the timestamp is provided (which used to be optional but now there is always a default value) it needs to be mapped. Its not a lot of work (for CPU) still could be avoided.

swift stdlib

It would not an issue if there was swift-server Swift Date type (DispatchWallTime ??), its Foundation.Date in iOS world

Proposal: How about we just drop the timestamp parameter all-together. If an implementation wants to allow mocking time it'd be up to a tracer's implementation, not to the API after all.

works for me, and its always easier to extend API than reduce it

having said that at times it may be useful to be able to "start span" before calling startSpan;
most likely very rare, and if there is a plan for "swift stdlib" perhaps TracingInstrument API could be extended then (?).

I found a use case for it when working on PoC of lambda:

I wanted to trace getNextInvocation BUT the trace context would be available for me in the invocation result.
So I need to remember when the operation started, but I will be able to record it using the proper trace context only
after I get the result.
(I updated XRay API to accommodate that and planning to put that to test today/tomorrow so there will be an example for that).

           // 1. start
            return self.runtimeClient.getNextInvocation(logger: logger).peekError { error in
                logger.error("could not fetch work from lambda runtime engine: \(error)")
            }.flatMap { invocation, event in
           // 2. invocation contains trace context, now we can record it