search

DataDog / dd-sdk-ios

Datadog SDK for iOS - Swift and Objective-C.
Apache License 2.0
205 stars 123 forks source link

Incorrect doc comment #2032

Closed liambutler-lawrence closed 3 weeks ago

liambutler-lawrence commented 4 weeks ago

Question

In DatadogInternal/Sources/NetworkInstrumentation/TraceID.swift, within struct DefaultTraceIDGenerator, the following constant declaration seems to have an incorrect doc comment:

    /// Describes the lower and upper boundary of tracing ID generation.
    ///
    /// * Lower: starts with `1` as `0` is reserved for historical reason: 0 == "unset", ref: dd-trace-java:DDId.java.
    /// * Upper: equals to `2 ^ 63 - 1` as some tracers can't handle the `2 ^ 64 -1` range, ref: dd-trace-java:DDId.java.
    public static let defaultGenerationRange = (1...UInt64.max)

Issues:

  1. UInt64.max, according to both the documentation and empirical testing, evaluates to 2 ^ 64 - 1 (18446744073709551615), not 2 ^ 63 - 1 (9223372036854775807).
  2. Also, the dd-trace-java package does not include any file named DDId.java.

I also noticed that in DatadogInternal/Sources/NetworkInstrumentation/SpanID.swift, within struct DefaultSpanIDGenerator there is a similar constant declaration that uses UInt64.max >> 1 (instead of just UInt64.max) which does evaluate to 2 ^ 63 - 1. Should that be the case in DefaultTraceIDGenerator as well?

Also related, both DefaultTraceIDGenerator and DefaultSpanIDGenerator use a repeat {} while {} loop to skip generated values of 0, but those are already eliminated since the range starts at 1, not 0. Isn't that unnecessary code then?

ganeshnj commented 3 weeks ago

hey @liambutler-lawrence

Indeed the docs are not up to date. I just a made a PR to reflect the changes.

For background, in past we have some tracers that couldn't handle the 2 ^ 63 hence we had to reduce the range by 1. This no longer the case with 128 bit ids and we can use the full range.