Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usage of deprecated instrumentation.Library in favor of Scope #3104

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

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

@Aneurysm9
Copy link
Member

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
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

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

Impacted file tree graph

@@           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

@dashpole
Copy link
Contributor

dashpole commented Aug 22, 2022

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.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Aug 22, 2022

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 golang/go@2580d0e

@Aneurysm9
Copy link
Member

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.

@bogdandrutu
Copy link
Member Author

@Aneurysm9 short answer is yes, the hypothesis on which the decision was made, is invalid, see #2977 (comment) which I am trying to prove that is not true, so the decision is wrong.

@Aneurysm9
Copy link
Member

@Aneurysm9 Aneurysm9 closed this Aug 22, 2022
@Aneurysm9
Copy link
Member

@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.

@Aneurysm9 Aneurysm9 reopened this Aug 22, 2022
@bogdandrutu
Copy link
Member Author

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.

@MadVikingGod
Copy link
Contributor

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.

@jmacd
Copy link
Contributor

jmacd commented Aug 24, 2022

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

@dashpole
Copy link
Contributor

dashpole commented Aug 24, 2022

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.

dmathieu added a commit that referenced this pull request Jul 24, 2024
…eprecation issues (#5627)

Rather than the deprecated InstrumentationLibrary

This is a replacement for
#3104, as after
this, there is no usage of `instrumentation.Library` within the SDK
anymore.

---------

Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Sam Xie <[email protected]>
@dmathieu
Copy link
Member

With #5627 merged, this PR can be closed. Please let me know if you feel I forgot something in there.

@dmathieu dmathieu closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants