Expose URLSessionInstrumentation configuration as public

[nhart12]

Follow up to #783

[ArielDemarco]

This change (together with the previous one) introduces potential thread safety issues, especially now that mutation is in play.

IMO, I would:

• Instead of marking this as public var, I’d mark it as private(set) var or leave it as is, so the object can’t be fully replaced from the outside. I assume the intent behind the original change was to allow replacing the tracer, but I doubt we want the entire URLSessionInstrumentationConfiguration to be replaced, as that could introduce even more subtle issues. Considering the next things i'll mention, i'd just leave it as is today (internal var configuration).
• Add a new public method that allows updating just the tracer (e.g. setTracer:). This method would access configuration internally and set a new tracer instance.
• Lastly, since tracer property may be accessed from multiple threads while another thread might be updating it using the method above, I’d make both read and write access thread-safe (either using a lock or a serial dispatch queue). Here’s a simple example using a serial queue:

  private let queue = DispatchQueue(label: "com.otel.urlInstrumentation.tracer")
  
  public var tracer: Tracer? {
      get { queue.sync { configuration.tracer } }
      set { queue.sync { configuration.tracer = newValue } }
  }

[nhart12]

great points.. In case users need to change the other configuration callback function references, do you think it makes sense to force all getters of the configuration to use a serial queue?

[ArielDemarco]

That’s a great point. If we expose the entire configuration publicly, then it could be a possibility. Since all properties in URLSessionInstrumentationConfiguration are currently public var, we open the door to potential race conditions across the board.

That said, up until now no one has really needed to mutate any existing configuration. The only use case that has come up is the one you brought that is replacing the tracer, which is why I focused on making access to that specific field thread-safe.

IMO, for now, I’d only wrap access to tracer, since it’s the only known mutable case and wrapping every single getter/setter would add unnecessary overhead and complexity to the capture logic (maybe we can measure if the overhead is significant?).

Longer term, I’d prefer to make all URLSessionInstrumentationConfiguration properties public private(set) and introduce explicit update methods if we need more mutability in the future; that would give us a cleaner and safer model overall.

LMK your thoughts @vvydier / @nachoBonafonte / @bryce-b on this

[bryce-b]

I think looking at expanding the configuration API to use some kind of resolver that can return the necessary tracer will provide better future utility.

Alternatively, would having the instrumentation rely only on the global tracer provider, rather than a member variable resolve this issue? Although, now that I look at it, registerTracerProvider isn't thread safe either...

I think wrapping the tracer access is a good interim solution while we discuss a long term solution.