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

Maven groupIds and artifactIds revisited #1186

Closed
trask opened this issue Sep 10, 2020 · 23 comments
Closed

Maven groupIds and artifactIds revisited #1186

trask opened this issue Sep 10, 2020 · 23 comments

Comments

@trask
Copy link
Member

trask commented Sep 10, 2020

Based on discussion from #1173 (comment) 😸:

io.opentelemetry.instrumentation

  • Library (manual) instrumentation that can be used by a user without the Java agent to instrument cats
    • opentelemetry-cats 🡆 opentelemetry-instrumentation-cats (for symmetry, though will create some long names)
  • Library (manual) instrumentation API
    • opentelemetry-instrumentation-api

io.opentelemetry.instrumentation.javaagent 🡆 io.opentelemetry.javaagent

  • Java agent (auto) instrumentation modules, loaded by the Java agent to interact with cats
    • opentelemetry-javaagent-cats
  • Java agent (auto) instrumentation API (TBD Decide on what modules contain public APIs #1129)
    • opentelemetry-javaagent-api
  • The Java agent itself
    • opentelemetry-javaagent
  • The Java agent SPI (TBD Decide on what modules contain public APIs #1129)
    • opentelemetry-javaagent-spi
  • Embedded exporters for the javaagent
    • opentelemetry-javaagent-exporters-otlp
    • opentelemetry-javaagent-exporters-jaeger
    • opentelemetry-javaagent-exporters-zipkin
    • opentelemetry-javaagent-exporters-logging
  • Internal components of the javaagent
    • opentelemetry-javaagent-bootstrap
    • opentelemetry-javaagent-tooling

The above naming also explores how things would look if we changed terminology from auto to javaagent.

@trask
Copy link
Member Author

trask commented Sep 10, 2020

I'm a little torn between auto and javaagent.

I agree auto is too generic.

On the other hand, javaagent is maybe too restrictive. We have already had discussions about re-using the bytecode instrumentation outside of a javaagent (e.g. bootstrapping during main() for aws lambda, and compile-time instrumentation for android).

OTOH, it makes sense to name things for their primary use cases (in our case, using javaagent instead of auto). I think it would be ok if we reuse those artifacts for other use cases (e.g. using javaagent artifacts for compile time android instrumentation).

@iNikem
Copy link
Contributor

iNikem commented Sep 10, 2020

I mostly have the same doubts. auto is too generic, but opentelemetry-javaagent is a mouthful to me. Also I think that primary use case is auto instrumentation (as in "non-manual"), and javaagent is kinda implementation detail. As demonstrated by our idea of static AOT instrumentation.

Apart from that, the list of names above does make sense.

@anuraaga
Copy link
Contributor

One reason to like javaagent instead of auto is spring-autoconfigure, which doesn't use our "auto instrumentation" modules is also basically auto instrumentation. But anything can be explained in a README too so don't have a strong preference, but a light preference for javaagent.

@trask
Copy link
Member Author

trask commented Sep 10, 2020

Also see https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/overview.md#instrumentation-libraries

Discussed with @anuraaga, and we think that the current naming meets the spec recommendation

the recommendation is to prefix packages with "opentelemetry-instrumentation", followed by the instrumented library name itself

In our case, prefixing the package is done via the groupId, e.g. io.opentelemetry.instrumentation:opentelemetry-grpc

@trask
Copy link
Member Author

trask commented Sep 11, 2020

Please have a look at this modified proposal:

io.opentelemetry.instrumentation

  • Library (manual) instrumentation that can be used by a user without the Java agent to instrument cats
    • instrumentation-cats
  • Library (manual) instrumentation API
    • instrumentation-api

(prefixing with instrumentation- instead of opentelemetry- creates nice symmetry/connection between instrumentation-cats and instrumentation-api, since we shouldn't use opentelemetry-api as artifactId)

io.opentelemetry.javaagent

(I think this makes the io.opentelemetry.instrumentation group clearer, not having javaagent nested inside of it)

  • Java agent (auto) instrumentation modules, loaded by the Java agent to interact with cats
    • javaagent-instrumentation-cats
  • Java agent (auto) instrumentation API (TBD Decide on what modules contain public APIs #1129)
    • javaagent-instrumentation-api
  • The Java agent itself
    • javaagent
  • The Java agent SPI (TBD Decide on what modules contain public APIs #1129)
    • javaagent-spi
  • Embedded exporters for the javaagent (maybe we don't need to publish?)
    • javaagent-exporters-otlp
    • javaagent-exporters-jaeger
    • javaagent-exporters-zipkin
    • javaagent-exporters-logging
  • Internal components of the javaagent (maybe we don't need to publish?)
    • javaagent-internal-bootstrap
    • javaagent-internal-tooling

@iNikem
Copy link
Contributor

iNikem commented Sep 11, 2020

I am concerned about the absence of opentelemetry prefixes on artifacts. E.g. if I have javaagent-spi.jar in my classpath, it is hard to understand that it is from OpenTelemetry.

And I read from spec, e.g. from here, that spec prefers to have opentelemetry prefix on artifacts.

Other than this prefix question, I support this proposal.

@anuraaga
Copy link
Contributor

Agree with @iNikem - during our call my thought was that we don't need -instrumentation in the artifacts since opentelemetry- is enough, and opentelemetry-instrumentation- is quite long, but I wouldn't drop opentelemetry from the artifacts completely.

@trask
Copy link
Member Author

trask commented Sep 14, 2020

Ya, good points, I agree.

So this?

io.opentelemetry.instrumentation

  • Library (manual) instrumentation that can be used by a user without the Java agent to instrument cats
    • opentelemetry-cats
  • Library (manual) instrumentation API
    • opentelemetry-instrumentation-api

io.opentelemetry.javaagent

  • Java agent (auto) instrumentation modules, loaded by the Java agent to interact with cats
    • opentelemetry-javaagent-cats
  • Java agent (auto) instrumentation API (TBD Decide on what modules contain public APIs #1129)
    • opentelemetry-javaagent-instrumentation-api
  • The Java agent itself
    • opentelemetry-javaagent
  • The Java agent SPI (TBD Decide on what modules contain public APIs #1129)
    • opentelemetry-javaagent-spi
  • Embedded exporters for the javaagent (maybe we don't need to publish?)
    • opentelemetry-javaagent-exporters-otlp
    • opentelemetry-javaagent-exporters-jaeger
    • opentelemetry-javaagent-exporters-zipkin
    • opentelemetry-javaagent-exporters-logging
  • Internal components of the javaagent (maybe we don't need to publish?)
    • opentelemetry-javaagent-internal-bootstrap
    • opentelemetry-javaagent-internal-tooling

@anuraaga
Copy link
Contributor

LGTM

@iNikem
Copy link
Contributor

iNikem commented Sep 14, 2020

This contradicts #1021, right? Should we close that one? @trask

@trask
Copy link
Member Author

trask commented Sep 21, 2020

Let's also change package names to match.

One nice thing about this is that everything in the javaagent will be under package io.opentelemetry.javaagent., which will guarantee it won't conflict with any library instrumentation which will be under completely separate package io.opentelemetry.instrumentation.

@trask
Copy link
Member Author

trask commented Oct 5, 2020

I'm planning to do the renames this week (before 0.9.0 release).

Since we are renaming things from auto --> javaagent, I think it makes sense to rename the auto (sibling of library) directory to javaagent also.

@trask
Copy link
Member Author

trask commented Oct 11, 2020

Trying out shortening the name opentelemetry-javaagent-instrumentation-api --> opentelemetry-javaagent-api

  • I think looks better in the docs as parallel to opentelemetry-instrumentation-api
  • I think also looks better in the docs as parallel to opentelemetry-javaagent-spi
  • I don't think causes confusion, because we have javaagent-spi and javaagent-api
  • I think reduces confusion, can talk about "instrumentation api" without confusion which "instrumentation api"
  • Package names become shorter io.opentelemetry.javaagent.api instead of io.opentelemetry.javaagent.instrumentation.api

Only down side I think is if we have other "javaagent apis" in the future(?)

@trask
Copy link
Member Author

trask commented Oct 11, 2020

  • Package names become shorter io.opentelemetry.javaagent.api instead of io.opentelemetry.javaagent.instrumentation.api

Not sure about this part ^^, e.g. it makes sense to me for

io.opentelemetry.javaagent.instrumentation.cats

to depend on

io.opentelemetry.javaagent.instrumentation.api

But we could still drop the -instrumentation from the artifact name, shortening it to opentelemetry-javaagent-api, which matches opentelemetry-javaagent-cats (where we already dropped the -instrumentation)

@trask
Copy link
Member Author

trask commented Oct 12, 2020

hm, maybe io.opentelemetry.javaagent.cats is ok (shorter) package name?

I kind of like that, as it reflects our goal to extract the "real"(?) instrumentation out into io.opentelemetry.instrumentation.cats, and then io.opentelemetry.javaagent.cats is the bytecode instrumentation that injects the "real"(?) instrumentation.

@trask
Copy link
Member Author

trask commented Oct 12, 2020

opentelemetry-javaagent-api will now have both "runtime" api that lives in bootstrap class loader and "matcher"/"bytebuddy" api that lives in agent class loader.

I'm thinking of naming those packages:

  • io.opentelemetry.javaagent.api.runtime
  • io.opentelemetry.javaagent.api.bytebuddy

.bytebuddy because it is already heavily dependent on bytebuddy api, and makes extra clear that this is the bytecode instrumentation portion used by bytebuddy vs the runtime portion

@iNikem
Copy link
Contributor

iNikem commented Oct 12, 2020

I kind of like that, as it reflects our goal to extract the "real"(?) instrumentation out into io.opentelemetry.instrumentation.cats, and then io.opentelemetry.javaagent.cats is the bytecode instrumentation that injects the "real"(?) instrumentation.

I see the allure of this but are we sure we will have a lot of library instrumentations? Because if we have only a handful of them, the argument above becomes weak IMO. And without that I feel unease with a long list of io.opentelemetry.javaagent.XXX as opposed to io.opentelemetry.javaagent.instrumentation.XXX

@trask
Copy link
Member Author

trask commented Oct 13, 2020

Summary after #1370 (still work to do):

groupId: io.opentelemetry.instrumentation

  • artifactId: opentelemetry-cats
    • package name: io.opentelemetry.instrumentation.cats
  • artifactId: opentelemetry-instrumentation-api
    • package name: io.opentelemetry.instrumentation.api

groupId: io.opentelemetry.javaagent.instrumentation

  • artifactId: opentelemetry-javaagent-cats
    • package name: io.opentelemetry.javaagent.instrumentation.cats

groupId: io.opentelemetry.javaagent

  • artifactId: opentelemetry-javaagent-api
    • package name: io.opentelemetry.javaagent.instrumentation.api
  • artifactId: opentelemetry-javaagent
  • artifactId: opentelemetry-javaagent-spi
  • artifactId: opentelemetry-javaagent-bootstrap (this still needs to go away)
  • artifactId: opentelemetry-javaagent-tooling (this still needs to go away)

@trask
Copy link
Member Author

trask commented Oct 13, 2020

I know I had proposed moving opentelemetry-javaagent-api to groupId io.opentelemetry.javaagent.instrumentation, but I thought more about it, and I think it makes sense to me under groupId io.opentelemetry.javaagent, and using package name io.opentelemetry.javaagent.api.

Then io.opentelemetry.javaagent.instrumentation is only for actual instrumentation.

Then the only artifact that doesn't really "fit" then is opentelemetry-instrumentation-api, and it makes sense that doesn't fit because

  • it can't be under groupId io.opentelemetry(?), so we force it to live in the same groupId the instrumentation that use it
  • it can't be named opentelemetry-api since that name conflicts with the real opentelemetry-api

So I propose keeping it as in above #1186 (comment) for 0.9.0, and shortly after I will work on moving the public api parts of javaagent-tooling into javaagent-api, and updating package names as proposed in #1186 (comment)

  • io.opentelemetry.javaagent.api.runtime
  • io.opentelemetry.javaagent.api.bytebuddy

@iNikem
Copy link
Contributor

iNikem commented Oct 14, 2020

I would like to point out that at least some of our problems with JFrog/Bintray/JCenter when people cannot find our published artifacts are due to JFrog being confused by our constant changes. E.g. Bintray->JCenter sync is not reliable because of that. Thus we really must stabilise our artifactIds ASAP to have them consumable by external users.

I would even go as far as claiming we should NOT publish any artifacts until we stabilise them to reduce confusion.

@trask trask unassigned trask Dec 7, 2020
@bogdandrutu
Copy link
Member

I have few questions about "javaagent" artifacts:

If groupId: io.opentelemetry.javaagent.instrumentation why do we repeat javaagent in the artifactId: opentelemetry-javaagent-cats? Same for groupId: io.opentelemetry.javaagent and artifactId: opentelemetry-javaagent-api?

@trask
Copy link
Member Author

trask commented Dec 9, 2020

If groupId: io.opentelemetry.javaagent.instrumentation why do we repeat javaagent in the artifactId: opentelemetry-javaagent-cats? Same for groupId: io.opentelemetry.javaagent and artifactId: opentelemetry-javaagent-api?

so that we won't produce the exact same jar file names for different artifacts which can be confusing

@trask
Copy link
Member Author

trask commented Apr 13, 2021

Closing, remaining items from above are being discussed now in #2779

@trask trask closed this as completed Apr 13, 2021
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

No branches or pull requests

4 participants