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

shouldn't have a hard-dependency on a <1.0 api #621

Open
codefromthecrypt opened this issue Mar 25, 2019 · 10 comments · Fixed by #834
Open

shouldn't have a hard-dependency on a <1.0 api #621

codefromthecrypt opened this issue Mar 25, 2019 · 10 comments · Fixed by #834
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@codefromthecrypt
Copy link

Even when not using tracing, opencensus 0.18 types are required per this line in HttpRequest

  /** OpenCensus tracing component. */
private final Tracer tracer = OpenCensusUtils.getTracer();

grpc code is more defensive to not require loading classes until used.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 25, 2019
@elharo
Copy link
Contributor

elharo commented Mar 26, 2019

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 30, 2019
@sduskis sduskis added type: question Request for information or clarification. Not an issue. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 11, 2019
@codefromthecrypt
Copy link
Author

why is this a question and not a bug?

@bogdandrutu
Copy link

OpenCensus versions <1.0 should be treated as any version >1.0. In the OpenCensus project we made a decision to respect all the requirements for versions >1.0 which means we do not break compatibility (remove classes/methods or change signatures).

So this should be closed.

@elharo
Copy link
Contributor

elharo commented Sep 29, 2019

If that's so, you might as well go ahead and release 1.0. However I have to note that for an allegedly stable product, the different versions of OpenCensus have caused us a lot of trouble.

@codefromthecrypt
Copy link
Author

Even if there is no argument about version drift etc, it is still completely unnecessary for this code to be called in absence of any setup instructions. 1.0 or not, it is pulling unnecessary dependencies into the application, and for no reason as the tracer isn't used.

@codefromthecrypt
Copy link
Author

the typical way to fix accidental classpath leak is to make a separate class that has the stuff inside. There are varying approaches to this. For example armeria recently made it possible to not have dropwizard in the classpath via this change line/armeria#2107

At some point, there might be some RPC library that you also find troublesomely hard pinned to something you don't want or use. I hope pointers on technical solution is helpful.

@chingor13 chingor13 added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Oct 2, 2019
@chingor13 chingor13 self-assigned this Oct 2, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Oct 2, 2019
@chingor13 chingor13 reopened this Oct 11, 2019
@codefromthecrypt
Copy link
Author

thanks, please note which version releases this so we can align deps!

@codefromthecrypt
Copy link
Author

PS It looks like the change was to remove OpenCensus. If that's something that needs to be reverted, I'd recommend having a look at the comment I mentioned here #621 (comment)

Basically, there are ways to perform the change in such a way that the census code still works, but is itself optional. If it isn't clear, and someone says "pull requests welcome" I would rather help implement that vs reverting back to having a strict dep on census again.

codyoss added a commit to codyoss/google-http-java-client that referenced this issue Oct 15, 2019
With these changes the user has the option to use opencensus. If
the deps are not on the classpath noop impls will be used. In the
future opencensus will be ripped out in favor of opentelemetry.

As a part of these changes OpenCensusUtils was moved to a
package-private scope.

Fixes: googleapis#621
codyoss added a commit to codyoss/google-http-java-client that referenced this issue Oct 15, 2019
With these changes the user has the option to use opencensus. If
the deps are not on the classpath noop impls will be used. In the
future opencensus will be ripped out in favor of opentelemetry.

As a part of these changes OpenCensusUtils was moved to a
package-private scope.

Fixes: googleapis#621
codyoss added a commit to codyoss/google-http-java-client that referenced this issue Oct 16, 2019
With these changes the user has the option to use opencensus. If
the deps are not on the classpath noop impls will be used. In the
future opencensus will be ripped out in favor of opentelemetry.

As a part of these changes OpenCensusUtils was moved to a
package-private scope.

Fixes: googleapis#621
@codyoss
Copy link
Member

codyoss commented Dec 9, 2019

I will leave this open, but this has been deprioritized. Down the road I think we might move towards using open telemetry. Until this, this will sit.

@codyoss codyoss removed the 🚨 This issue needs some love. label Dec 9, 2019
@codyoss codyoss added priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Dec 9, 2019
@elharo
Copy link
Contributor

elharo commented Dec 12, 2019

If we're not ripping out OpenCensus, we might want to at least exclude the paths to it's older dependencies, especially Guava:

+-c:qa:0.1.0-SNAPSHOT
  +-com.google.api:gax:1.51.0
    +-com.google.auth:google-auth-library-oauth2-http:0.18.0
      +-com.google.http-client:google-http-client:1.32.1
        +-com.google.guava:guava:28.1-android
and
+-c:qa:0.1.0-SNAPSHOT
  +-com.google.api:gax:1.51.0
    +-com.google.auth:google-auth-library-oauth2-http:0.18.0
      +-com.google.http-client:google-http-client:1.32.1
        +-io.opencensus:opencensus-contrib-http-util:0.24.0
          +-com.google.guava:guava:26.0-android

see googleapis/gax-java#662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
7 participants