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

Gradle dependencies cleanup #556

Merged
merged 11 commits into from
Jun 24, 2020
Merged

Gradle dependencies cleanup #556

merged 11 commits into from
Jun 24, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Jun 22, 2020

Partially adresses #351 .

This PR touches mostly "shared" modules, like auto-bootstrap or java.gradle to allow for meaningful review. Later PR will change "leave" modules, where changes are very mechanical.

@iNikem iNikem changed the title Warnings Gradle dependencies cleanup Jun 22, 2020
Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always had a hard time understanding when to use gradle api vs implementation. Thanks for cleaning this up.

@iNikem
Copy link
Contributor Author

iNikem commented Jun 22, 2020

But I don't understand why muzzle fails :(

@trask
Copy link
Member

trask commented Jun 23, 2020

I think muzzle found a real issue. Not really sure why it didn't catch it before, probably your dependency cleanup revealed it somehow.

To make muzzle pass for now, go ahead and lie to it:

muzzle {
  pass {
    group = "io.opentracing.contrib.dropwizard"
    module = "dropwizard-opentracing"
    versions = "(,)"
    extraDependency 'io.opentelemetry:opentelemetry-contrib-auto-annotations:0.5.0'
  }
}

i think the "correct" way to deal with this is to split out the WithSpan instrumentation into a separate module, e.g. trask@a8e7e82

i'll turn this prototype into a follow-on PR after this one is merged

@iNikem
Copy link
Contributor Author

iNikem commented Jun 23, 2020

@trask can you explain why current approach does not work? What is wrong with it?

@trask
Copy link
Member

trask commented Jun 23, 2020

WithSpan should be treated like a third-party library that we are instrumenting, e.g.

https://github.com/trask/opentelemetry-java-instrumentation/blob/split-annotation-module/instrumentation/annotations/annotations-opentelemetry/annotations-opentelemetry.gradle#L4-L21

but instead we have a weird mix of treating it as a class we are instrumenting, but also as an internal class, e.g. we add it to the bootstrap classes in SpockRunner.java which is incorrect, and we relocate it in java-agent.gradle which is also incorrect.

@iNikem
Copy link
Contributor Author

iNikem commented Jun 23, 2020

I see... Ok, I mostly understand, but why we have to split annotations instrumentation module? If we should treat WithSpan as any other third party library, why should be it separate from other annotations support?

@trask
Copy link
Member

trask commented Jun 24, 2020

Oh, I think this is the way to go:

muzzle {
  pass {
    group = "io.opentelemetry"
    module = "opentelemetry-contrib-auto-annotations"
    versions = "(,)"
    extraDependency 'io.opentracing.contrib.dropwizard:dropwizard-opentracing:0.2.2'
  }
}

I was going to say it's better when there's only a single compile-time dependency in each instrumentation module (and that's the dependency we put in the muzzle block).

But then I realized that the trace-annotation module already only has one compile-time dependency, since WithSpan is the only annotation we take a compile-time dependency on.

We still need the extraDependency, because otherwise muzzle will say that TraceAnnotationsInstrumentation failed to match anything on the classpath.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turned my comment above into a suggestion below

instrumentation/trace-annotation/trace-annotation.gradle Outdated Show resolved Hide resolved
@iNikem iNikem merged commit 9a52f67 into open-telemetry:master Jun 24, 2020
@iNikem iNikem deleted the warnings branch June 24, 2020 09:01
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.

3 participants