Avoid manual artifact/archive renaming (composite build readiness) #3603

Oberon00 · 2021-09-10T17:29:29Z

Instead of using tricks to be able to use shorter project names, just name the folders like we want the final published artifact to be named.
This should enable composite builds to use opentelemetry-java without hassle, but I'm currently still verifying this (there seems to be an issue with the substituted bom or similar).

Prerequisite for open-telemetry/opentelemetry-java-instrumentation#3626 (CC @iNikem)

Oberon00 · 2021-09-10T17:35:30Z

buildSrc/src/main/kotlin/otel.publish-conventions.gradle.kts

      afterEvaluate {
        // not available until evaluated.
-        artifactId = base.archivesName.get()


Getting rid of this and ensuring that project name == archive name == artifact ID was key here.

codecov · 2021-09-10T17:42:22Z

Codecov Report

Merging #3603 (048d734) into main (39f3362) will decrease coverage by 0.12%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3603      +/-   ##
============================================
- Coverage     88.55%   88.43%   -0.13%     
- Complexity     3571     3654      +83     
============================================
  Files           390      404      +14     
  Lines         11687    11961     +274     
  Branches       1156     1186      +30     
============================================
+ Hits          10350    10578     +228     
- Misses          968     1004      +36     
- Partials        369      379      +10

Impacted Files	Coverage Δ
...a/io/opentelemetry/sdk/trace/internal/JcTools.java	`33.33% <ø> (ø)`
...rter/otlp/http/metrics/OtlpHttpMetricExporter.java	`85.45% <0.00%> (-7.28%)`	⬇️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java	`94.11% <0.00%> (-5.89%)`	⬇️
...extension/aws/resource/LambdaResourceProvider.java	`50.00% <0.00%> (ø)`
...emetry/sdk/extension/aws/resource/EcsResource.java	`78.94% <0.00%> (ø)`
...ension/aws/resource/BeanstalkResourceProvider.java	`50.00% <0.00%> (ø)`
...etry/sdk/extension/aws/internal/JdkHttpClient.java	`87.50% <0.00%> (ø)`
...emetry/sdk/extension/aws/resource/EksResource.java	`75.00% <0.00%> (ø)`
...k/extension/aws/resource/AwsResourceConstants.java	`100.00% <0.00%> (ø)`
...metry/sdk/extension/aws/resource/DockerHelper.java	`87.50% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f3362...048d734. Read the comment docs.

Oberon00 · 2021-09-13T14:08:14Z

buildSrc/src/main/kotlin/otel.java-conventions.gradle.kts

@@ -175,7 +167,7 @@ dependencies {
  dependencyManagement(platform(project(":dependencyManagement")))
  afterEvaluate {
    configurations.configureEach {
-      if (isCanBeResolved && !isCanBeConsumed) {
+      if (!isCanBeConsumed && this != dependencyManagement) {


This change is required for the composite build to work:

So this is not enough to make composite builds work as there is at least one other problem, namely how dependency management is set up. The constraints seem to be only applied if the ultimately built project has the constraints applied. For example, without this change, if I try inside the examples subdirectory:

$ ./gradlew classes --include-build .. ... FAILURE: Build failed with an exception. * What went wrong: Could not determine the dependencies of task ':opentelemetry-examples-zipkin:compileJava'. > Could not resolve all task dependencies for configuration ':opentelemetry-examples-zipkin:compileClasspath'. > Could not find io.zipkin.reporter2:zipkin-reporter:. Required by: project :opentelemetry-examples-zipkin > project :odin-java:exporters:opentelemetry-exporter-zipkin

We can see that opentelemetry-exporter-zipkin has the dependency api("io.zipkin.reporter2:zipkin-reporter"). However, the api configuration extends from nothing, so it does not have dependency management applied, and opentelemetry-examples-zipkin:compileClasspath does not use the java-conventions plugin so it does not have dependency management applied either.

@anuraaga Actually I'll need to resurrect this one change for the included build to actually work. Seems I tested with some changes from here mixed in when I made #3653.

Oberon00 · 2021-09-13T15:21:47Z

buildSrc/src/main/kotlin/otel.publish-conventions.gradle.kts

 plugins {
  `maven-publish`
  signing

  id("otel.japicmp-conventions")
 }

+base {


Moved from buildSrc/src/main/kotlin/otel.java-conventions.gradle.kts

Oberon00 · 2021-09-13T15:22:27Z

...ples/sdk-usage/src/main/java/io/opentelemetry/sdk/example/ConfigureSpanProcessorExample.java

@@ -25,6 +27,9 @@
  private static final Tracer tracer = openTelemetry.getTracer("ConfigureSpanProcessorExample");

  public static void main(String[] args) {
+    System.out.println(


I added this to test the included build, I can remove or adapt it if desired.

Oberon00 · 2021-09-13T15:23:50Z

settings.gradle.kts

+include(":sdk-extensions:opentelemetry-sdk-extension-jfr-events")
+include(":sdk-extensions:opentelemetry-sdk-extension-zpages")
+
+// Map project names to directory names.


I added this instead of just renaming the folders for now. I would prefer renaming the folders but that makes the PR huge (1070 changed files). Can do in a follow-up.

anuraaga · 2021-09-14T02:05:07Z

As someone who writes a lot of code in this repo, the longer names will definitely hurt the contributor experience I think. Publishing snapshots usually works easily enough and I don't think we've had as much need to write -instrumentation code against HEAD -java code since releasing 1.0. The linked issue problem about gradle-plugins will probably be handled without using a composite build open-telemetry/opentelemetry-java-instrumentation#4112

Composite builds have been around for a while but the limitations seem to not be getting fixed - this makes me think the feature is not very popular.

So I'm not too enthusastic about this change TBH.

Oberon00 · 2021-09-14T07:28:24Z

I was thinking that it is odd that the artifact IDs start with opentelemetry- despite already having a group of io.opentelemetry. Renaming our artifacts would be one way to fix the names. We could publish POM-only artifacts with an api dependency on the new name under the old name.

Also, for me, right now I always need to squint hard to find the artifact name for any given project.

Oberon00 · 2021-09-14T07:32:11Z

As someone who writes a lot of code in this repo, the longer names will definitely hurt the contributor experience I think.

Would the typical contributor write much Gradle code?

Oberon00 · 2021-09-14T10:32:36Z

That makes sense. In that case, it might be possible to rename just the jar but not the artifact ID? But I don't see the problem with long project names anyway and would prefer to avoid any extra configuration if possible, since that makes everything more complex.

iNikem · 2021-09-14T10:50:49Z

I agree that simplicity of having project names equal to the desired artifact names and GAV coordinates is important enough to accept longer folder names during development.

anuraaga · 2021-09-14T11:10:49Z

Would the typical contributor write much Gradle code?

The typical contributor will run many Gradle tasks which is what gets more tedious.

I agree that simplicity of having project names equal to the desired artifact names and GAV coordinates is important enough to accept longer folder names during development.

We should use the same pattern in our three repos for sanity - so ./gradlew :instrumentation:opentelemetry-api:opentelemetry-api-1.0:opentelemetry-javaagent-instrumentation-api-1.0:check? :P

Flattening into just ./gradlew :opentelemetry-javaagent-instrumentation-api-1.0:check is sort of ok in isolation, but has its own problems (javaagent library testing artifacts get randomly placed).

Losing control of your folder structure seems like a reasonable downside that is being missed. I'm still a -1 vote on it.

Oberon00 · 2021-09-14T11:49:45Z

The typical contributor will run many Gradle tasks which is what gets more tedious.

That's a point. Counterpoint: I usually don't run tasks per project (incremental builds are great!) or I use the command line history, so the longer names wouldn't bother me. Also, you can abbreviate kebap-case names: https://docs.gradle.org/current/userguide/command_line_interface.html#sec:name_abbreviation E.g. ./gradlew :exp:oEP:classes actually executes :exporters:opentelemetry-exporter-prometheus:classes (just tested)

Oberon00 · 2021-09-14T11:55:19Z

./gradlew :instrumentation:opentelemetry-api:opentelemetry-api-1.0:opentelemetry-javaagent-instrumentation-api-1.0:check

Should be doable with ./gradlew :instrumentation:oA:oA-1.0:oJIA-1.0:check (using oJavaagentIA or variants should also work)

anuraaga · 2021-09-15T02:16:09Z

./gradlew :instrumentation:oA:oA-1.0:oJIA-1.0:check - I guess this sort of works (personally it takes so long to generate kebab case shorthand in my head but admittedly should be able to get used to it).

We're still losing control of our folder naming scheme, which I think is a big enough downside. Basically we have to decide whether we accept tradeoffs to support composite builds - given that these bugs have been long-standing within Gradle, my inclination is not to support them. Publishing to maven local tends to work well enough for those times when doing cross-repo development, which I don't think happens that much anymore.

While we theoretically can apply this pattern to the instrumentation repo, I'm pretty sure we wouldn't. I think compared to this repo, there it's just way too big of a regression compared to our current, at least IMO, easy to understand structure of three folders per instrumentation clearly being subcomponents of that project.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/grpc-1.6

Currently 99% of the build logic is shared among the repos and I'm interested in publishing a Gradle plugin soon so we can actually use just one source for them. This change I honestly expect to toss a big wrench into that, unless we really do apply the naming to the instrumentation repo, which seems like a loss.

I am -1 for this still, but wouldn't block if there are enough votes otherwise and someone is committed to applying the pattern across the three repos. @trask @jkwatson please share your thoughts :)

jkwatson · 2021-09-15T02:39:22Z

I'm against this change unless there is a really compelling reason for it. I haven't heard a very compelling reason articulated as of yet.

trask · 2021-09-15T02:46:10Z

@trask @jkwatson please share your thoughts :)

I added to next week Tue SIG meeting agenda so we can discuss pros/cons of applying to the instrumentation repo with @iNikem since he opened the original issue.

Oberon00 · 2021-09-15T07:41:13Z

As an alternative to this, there is also the possibility to declare substitutions in the consuming project: https://docs.gradle.org/current/userguide/composite_builds.html#included_build_declaring_substitutions
Maybe there would be some way to provide some .gradle or .gradle.kts file that could be included by the consumer to enable composite builds.

@anuraaga
Composite builds have been around for a while but the limitations seem to not be getting fixed - this makes me think the feature is not very popular.

Basically we have to decide whether we accept tradeoffs to support composite builds - given that these bugs have been long-standing within Gradle, my inclination is not to support them.

The alternative conclusion that nobody changes the artifact ID, so nobody is affected by this limitation would be just as valid. At Dynatrace we use composite builds extensively for our larger repositories.

Maybe changing just the JAR name but not including opentelemetry- in the artifact ID would keep the composite build working.

iNikem · 2021-09-15T10:54:32Z

What about keeping the folder structure but changing project names? Not artifacts names/coordinates, but sub-projects names? Just an idea, I haven't tried it.

Oberon00 · 2021-09-15T11:31:31Z

What about keeping the folder structure but changing project names?

That's already what is being done here. The issue @anuraaga has are the task names, not the folder names, as far as I understand.

anuraaga · 2021-09-15T12:26:30Z

Keeping folder names the same as the task path is the #1 priority. It's the only thing I would try to block regardless of a vote - it's just too confusing to know how to run commands if you can't trust the folder structure. This repo started with that, it was so hard.

The scheme I am still fairly receptive to is a 100% flat structure - all project folders are at the top level. I think it's possible to apply to this repo, but I think it's impossible to apply to the instrumentation repo because our artifact names do not sort well alphabetically there, and we have tons of unpublished projects. Nor should they have to - this is a Gradle bug a PR to fix Gradle would have no tradeoffs :-D

anuraaga · 2021-09-15T13:16:46Z

BTW filed gradle/gradle#18291

Oberon00 · 2021-09-15T13:35:04Z

Keeping folder names the same as the task path is the #1 priority.

I agree that this is desirable. This PR only lets them diverge because you would otherwise have 1070 renamed files to review and the GH UI becomes very slow (see earlier PR versions).

anuraaga · 2021-09-15T14:11:59Z

Yup I got that, this repo by itself we could then converge in the next PR on just the flat structure. I don't think it scales to repos like the instrumentation repo though - because we have such repos (large monorepos) we should take it into account. Loosing consistency among the repos is of course an option - but as possibly the only one who is proactive in all three it would be quite painful for me.

jkwatson · 2021-09-15T14:45:55Z

Whatever happens here, we must not change the artifact names or maven coordinates of anything that we have marked stable at this point.

Oberon00 · 2021-09-21T09:32:42Z

Doesn't look like a gradle-side solution is coming 😕 gradle/gradle#18291 was closed as duplicate of the very old gradle/gradle#847.

If the main concern is gradle task names, maybe a task rule could be used to dispatch tasks like :instrumentation-apache-camel-2.20-javaagent to :instrumentation:apache-camel-2.20:opentelemetry-instrumentation-apache-camel-2.20-javaagent (not sure how the current project name actually is). Though personally I think I would be content just using command line history or an IDE or the kebap abbreviations

iNikem · 2021-09-21T09:59:28Z

Actually, I now learned that there is a way to solve my original problem in open-telemetry/opentelemetry-java-instrumentation#3626. Yes, automatic mapping of included project's modules to maven coordinates does not work for our repos atm. But you can manually specify dependency substitution rules as in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/4179/files#diff-0b68cb406e55a2431167c05be6ceb1c7e64c13ef9f460dc0b9c80cc860a2ea7d. It's not ideal, but it works.

Oberon00 · 2021-09-21T10:17:09Z

How does this .using(project(":")) work? Does : refer to the current project or the root project? Wouldn't you need to specify the exact subproject?

iNikem · 2021-09-21T15:43:01Z

How does this .using(project(":")) work? Does : refer to the current project or the root project? Wouldn't you need to specify the exact subproject?

using select a project from the included build. In the referenced case we include gradle-plugin project which does not have any subprojects.

Oberon00 · 2021-09-21T18:29:28Z

OK, so you basically have to spell out for gradle what the publish configuration does in a different format.
I wonder if it would be possible to generate a dependencySubstitution for use in other projects automatically from open-telemetry java.

iNikem · 2021-09-21T19:56:15Z

OK, so you basically have to spell out for gradle what the publish configuration does in a different format.
I wonder if it would be possible to generate a dependencySubstitution for use in other projects automatically from open-telemetry java.

If we have just to strip opentelemetry- prefix, then sure :)

Oberon00 · 2021-09-22T17:00:48Z

If we have just to strip opentelemetry- prefix, then sure :)

We also have to replace the group and sometimes change singular to plural and I think for nested exporter packages you might need special handling. I was more thinking of doing this as part of the publish-conventions plugin, having a task that depends-on everything published that then can access the final publish configuration.

You can see that the mapping I do from project names to paths currently in settings.gradle is not reversible (since I try both prefix and singularPrefix)

Oberon00 · 2021-10-07T12:02:42Z

Closing this one as there seems to be no realistic chance of it getting merged (especially after #3653). Maybe if something improves on the Gradle-side this would become interesting again.

Oberon00 mentioned this pull request Sep 10, 2021

Unable to use composite build open-telemetry/opentelemetry-java-instrumentation#3626

Closed

This comment has been minimized.

Sign in to view

Oberon00 commented Sep 10, 2021

View reviewed changes

Oberon00 changed the title ~~Dumb down project naming.~~ Rename folders to avoid manual artifact/archive renaming (composite build readiness) Sep 13, 2021

Oberon00 commented Sep 13, 2021

View reviewed changes

Oberon00 force-pushed the kiss-project-names branch from da625fc to 9e69dba Compare September 13, 2021 15:20

Oberon00 commented Sep 13, 2021

View reviewed changes

Oberon00 changed the title ~~Rename folders to avoid manual artifact/archive renaming (composite build readiness)~~ Avoid manual artifact/archive renaming (composite build readiness) Sep 13, 2021

Make project names match artifact IDs.

f136531

Oberon00 force-pushed the kiss-project-names branch from 9e69dba to f136531 Compare September 13, 2021 15:29

Oberon00 added 3 commits September 13, 2021 17:34

Debug CI.

bc7cd03

Fix leading slash.

8781a23

Remove debug println.

048d734

Oberon00 marked this pull request as ready for review September 13, 2021 15:54

Oberon00 requested review from anuraaga, arminru, bogdandrutu, carlosalberto and jkwatson as code owners September 13, 2021 15:54

Oberon00 requested a review from a user September 13, 2021 15:54

Oberon00 requested review from pavolloffay, thisthat and tylerbenson as code owners September 13, 2021 15:54

Oberon00 requested a review from iNikem September 13, 2021 15:55

anuraaga mentioned this pull request Sep 15, 2021

Allow projects to have aliases gradle/gradle#18291

Closed

Oberon00 mentioned this pull request Sep 17, 2021

No documentation on artifact names (or it is not easy to find) #3632

Closed

Oberon00 mentioned this pull request Sep 23, 2021

Add generateBuildSubstitutions task to bom projects. #3653

Merged

Oberon00 closed this Oct 7, 2021

Oberon00 deleted the kiss-project-names branch October 7, 2021 12:02

Oberon00 mentioned this pull request Oct 7, 2021

Apply dependencyManagement also to unresolvable configurations for composite build #3706

Closed

Avoid manual artifact/archive renaming (composite build readiness) #3603

Avoid manual artifact/archive renaming (composite build readiness) #3603

Conversation

Oberon00 commented Sep 10, 2021 • edited Loading

This comment has been minimized.

Oberon00 Sep 10, 2021

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2021 • edited Loading

Codecov Report

Oberon00 Sep 13, 2021

Choose a reason for hiding this comment

Oberon00 Oct 7, 2021

Choose a reason for hiding this comment

Oberon00 Sep 13, 2021

Choose a reason for hiding this comment

Oberon00 Sep 13, 2021

Choose a reason for hiding this comment

Oberon00 Sep 13, 2021

Choose a reason for hiding this comment

anuraaga commented Sep 14, 2021

Oberon00 commented Sep 14, 2021 • edited Loading

Oberon00 commented Sep 14, 2021

Oberon00 commented Sep 14, 2021

iNikem commented Sep 14, 2021

anuraaga commented Sep 14, 2021

Oberon00 commented Sep 14, 2021 • edited Loading

Oberon00 commented Sep 14, 2021 • edited Loading

anuraaga commented Sep 15, 2021 • edited Loading

jkwatson commented Sep 15, 2021 • edited Loading

trask commented Sep 15, 2021

Oberon00 commented Sep 15, 2021

iNikem commented Sep 15, 2021

Oberon00 commented Sep 15, 2021 • edited Loading

anuraaga commented Sep 15, 2021 • edited Loading

anuraaga commented Sep 15, 2021

Oberon00 commented Sep 15, 2021 • edited Loading

anuraaga commented Sep 15, 2021

jkwatson commented Sep 15, 2021

Oberon00 commented Sep 21, 2021 • edited Loading

iNikem commented Sep 21, 2021

Oberon00 commented Sep 21, 2021

iNikem commented Sep 21, 2021

Oberon00 commented Sep 21, 2021

iNikem commented Sep 21, 2021

Oberon00 commented Sep 22, 2021 • edited Loading

Oberon00 commented Oct 7, 2021 • edited Loading

Oberon00 commented Sep 10, 2021 •

edited

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

Oberon00 commented Sep 14, 2021 •

edited

Loading

Oberon00 commented Sep 14, 2021 •

edited

Loading

Oberon00 commented Sep 14, 2021 •

edited

Loading

anuraaga commented Sep 15, 2021 •

edited

Loading

jkwatson commented Sep 15, 2021 •

edited

Loading

Oberon00 commented Sep 15, 2021 •

edited

Loading

anuraaga commented Sep 15, 2021 •

edited

Loading

Oberon00 commented Sep 15, 2021 •

edited

Loading

Oberon00 commented Sep 21, 2021 •

edited

Loading

Oberon00 commented Sep 22, 2021 •

edited

Loading

Oberon00 commented Oct 7, 2021 •

edited

Loading