Signed-off-by: Bogdan Drutu [email protected]

instrumentation.Library is now aliased to instrumentation.Scope. I'm not sure that we should be changing the types of parameters and return values in stable modules. @open-telemetry/go-maintainers thoughts?

Codecov Report

Merging #3104 (72853b8) into main (8c3a85a) will decrease coverage by 0.0%. The diff coverage is 61.9%.

@@           Coverage Diff           @@
##            main   #3104     +/-   ##
=======================================
- Coverage   76.2%   76.2%   -0.1%     
=======================================
  Files        179     179             
  Lines      11942   11942             
=======================================
- Hits        9110    9106      -4     
- Misses      2592    2596      +4     
  Partials     240     240             

Impacted Files	Coverage Δ
...rs/otlp/otlpmetric/internal/otlpmetrictest/data.go	0.0% <0.0%> (ø)
...ters/otlp/otlptrace/internal/otlptracetest/data.go	0.0% <0.0%> (ø)
sdk/metric/export/metric.go	100.0% <ø> (ø)
sdk/trace/snapshot.go	91.4% <0.0%> (ø)
sdk/trace/tracetest/span.go	93.5% <0.0%> (ø)
exporters/prometheus/prometheus.go	65.6% <50.0%> (ø)
sdk/metric/processor/processortest/test.go	84.0% <66.6%> (ø)
bridge/opencensus/exporter.go	100.0% <100.0%> (ø)
...otlp/otlpmetric/internal/metrictransform/metric.go	78.1% <100.0%> (ø)
exporters/stdout/stdoutmetric/metric.go	81.7% <100.0%> (ø)
... and 5 more

Yeah, I don't think this would be backwards-compatible, so we will probably have to exclude uses in stable modules. I'm in favor of it elsewhere, though.

Yeah, I don't think this would be backwards-compatible

How come is not? Type alias does NOT create a new type, so the 2 "types" are equal, hence the generated code is backwards compatible.

To prove my point, see https://github.com/golang/go/commit/2580d0e08d5e9f979b943758d3c49877fb2324cb

interface{} and any are perhaps not the best types to use to illustrate the point since they're explicitly intended to match any type and it would be impossible to break compatibility by exchanging them. Are there any examples with concrete types that are not referenced in the language spec?

We have previously decided not to make this change. I don't see anything that changes that decision.

@Aneurysm9 short answer is yes, the hypothesis on which the decision was made, is invalid, see https://github.com/open-telemetry/opentelemetry-go/pull/2977#discussion_r907491454 which I am trying to prove that is not true, so the decision is wrong.

https://go.dev/play/p/Yt9aOTDzJCI

@MadVikingGod pointed out that we used an alias rather than a typedef and I have bad memory.

That said, we have a goal of ensuring that dependency hell will never prevent a user from updating their instrumentation or SDK, I'd prefer to take the safer route and not change these uses in stable packages.

That said, we have a goal of ensuring that dependency hell will never prevent a user from updating their instrumentation or SDK, I'd prefer to take the safer route and not change these uses in stable packages.

Unless I am missing something obvious, this will break only if only users are forcing an older SDK. But that is a problem that you have even if for example you add a new func in API, that SDK uses, you should never use that.

I think we may need a bit more deliberate approach to this than what has been offered here. The metrics are fine, that is not under the 1.x stability, but I think the tracing story is different.

The two changes I would focus on are:

1. Changing ReadOnlySpan from InstrumentationLibrary() instrumentation.Library to InstrumentationLibrary() instrumentation.Scope. This function is already deprecated, maybe we could do better deprecating the implementations, but I don't think it sends the right signal when the Name and the struct don't follow the rest methods. Yes, it can be done, but it needs to be noted why it's different so that someone that was not around for the library scope transition has the context they need.
2. Changing the tracetest.SpanStub from InstrumentationLibrary instrumentation.Library to InstrumentationLibrary instrumentation.Scope. For the same reasons as above, I don't think this is good to change without commenting on why. I also don't think this will be as clean as a change because we can't just add an InstrumentationScope field like we did with the Interface. This would be a longer approach of creating a replacement stub, transitioning to it, and deprecating the old.

I am confused as to why type aliases do not solve the problem. The same play link with an alias compiles and works as intended, right? https://go.dev/play/p/TQukM9rrXQd @Aneurysm9

Thanks for explaining @jmacd. I agree that adding brief comments for when the function name and returned type differ would help clarify. But i'm still in favor of making the changes.