Change the default javaagent artifact to have exporters #4106

trask · 2021-09-13T00:27:54Z

And introduce a new artifact with classifier "slim" ~~which does not have any exporters (same as what used to be the default artifact).~~ EDIT: which has only otlp/grpc (and using okhttp instead of netty for size)

Resolves #3959

Will send separate PR to be merged after the release updating:

anuraaga · 2021-09-13T02:37:41Z

.github/workflows/patch-release-build.yml

-          asset_path: javaagent/build/libs/opentelemetry-javaagent-${{ github.event.inputs.version }}-all.jar
-          asset_name: opentelemetry-javaagent-all.jar
+          asset_path: javaagent/build/libs/opentelemetry-javaagent-${{ github.event.inputs.version }}.jar
+          asset_name: opentelemetry-javaagent.jar


I agree with the rename but it's sort of a breaking change for e.g., Dockerfiles that may be curling the "latest release" URL. Should we upload both at least for one release?

We may also consider whether it is bad practice in general to point at an unversioned URL - if so we can probably update our README to not point at the latest version URL to discourage usage.

Should we upload both at least for one release?

makes sense to me, I added attaching backwards compatible "all" artifact step to both release and patch release builds

update our README to not point at the latest version URL to discourage usage

I like explicit versions in docs (and keeping it updated), IIRC we were stuck on how to automate this? maybe we can tack this problem on to #3516 @iNikem?

docs/manual-instrumentation.md

trask · 2021-09-13T04:45:29Z

benchmark-overhead/src/test/java/io/opentelemetry/agents/LatestAgentSnapshotResolver.java

+          // TODO (trask) this needs to be updated now that no more "all" artifact
          return "all".equals(classifier) && "jar".equals(extension);


@breedx-splk any idea with this change if the classifier check should be changed to "" or null?

Is the idea here that we'll be publishing the full agent with exporters now without the -all classifier? If so, then I think we should just remove the classifier check entirely.

It looks like you're also publishing -all for backwards compat? Assuming that's temporary, yeah, I think we'll want to remove this classifier check...but it might be nice to see what the xml looks like after the first publish to confirm.

the -all artifact will be attached to the github release for now for backwards compatibility, but won't be published.

there will be a new artifact -slim published after this tho

it might be nice to see what the xml looks like after the first publish to confirm

this works for me 👍

iNikem · 2021-09-13T09:01:25Z

I vaguely remember that we wanted to make this change after extracting contrib portion. To minimize the number of times we change the meaning of our artifacts.

…agent-to-all

trask · 2021-09-13T17:50:57Z

I vaguely remember that we wanted to make this change after extracting contrib portion. To minimize the number of times we change the meaning of our artifacts.

I added to tomorrow's meeting agenda

trask · 2021-09-14T05:43:34Z

I vaguely remember that we wanted to make this change after extracting contrib portion. To minimize the number of times we change the meaning of our artifacts.

I added to tomorrow's meeting agenda

moving back to draft for now and deferring until after the 1.6.0 release

…agent-to-all

trask · 2021-09-23T20:21:17Z

moving back to draft for now and deferring until after the 1.6.0 release

we discussed in SIG meetings this week, and I think there's general agreement to go forward with this PR now (see discussion in #3959)

I'll raise once more in next Tue SIG meeting and will merge after that if there's still agreement.

…agent-to-all

trask · 2021-09-23T21:24:30Z

and will add otlp/http exporter to "-slim" as a follow-up PR (for now in this PR "-slim" has no exporters, same as the previous default artifact)

…agent-to-all

anuraaga · 2021-09-29T04:57:11Z

javaagent/build.gradle.kts

@@ -73,6 +78,10 @@ dependencies {

  exporterLibs(project(":javaagent-exporters"))

+  exporterSlimLibs("io.opentelemetry:opentelemetry-exporter-otlp")
+  exporterSlimLibs("io.opentelemetry:opentelemetry-exporter-otlp-metrics")
+  exporterSlimLibs("io.grpc:grpc-okhttp:1.41.0")


Just realized that because this is using okhttp, adding the HTTP exporters as well should be almost no additional bytecode, so maybe worth it?

Well assuming it doesn't cause dependency hell, wow that's old

https://github.com/grpc/grpc-java/blob/master/build.gradle#L158

Err - nevermind, yeah it wouldn't cause dependency hell since it's a different artifact

https://github.com/open-telemetry/opentelemetry-java/blob/main/dependencyManagement/build.gradle.kts#L78

So scratch the almost-no-additional-bytecode idea.

By the way, just some random trivia grpc/grpc-java#6119 (comment)

"OkHttp" name for the transport is a bit of a misnomer

this thread helped my mental picture 👍

It appears that we can exclude okhttp 2 from the transitive dependencies

open-telemetry/opentelemetry-java#3678

though I added a comment on the grpc-java issue to see if there is anything obviously bad with that.

Currently I guess we effectively have two copies of okhttp2, the forked-into-grpc one and the transitive dependency which we don't seem to need.

I added #4240 to discuss/track

…agent-to-all

iNikem · 2021-09-29T07:31:42Z

docs/manual-instrumentation.md

-> :warning: prior to version 1.0.0, `opentelemetry-javaagent-all.jar`
-only supports manual instrumentation using the `opentelemetry-api` version with the same version
-number as the Java agent you are using. Starting with 1.0.0, the Java agent will start supporting
-multiple (1.0.0+) versions of `opentelemetry-api`.
-


Are we sure about that? Have we tested that?

we have a smoke test that verifies interop with opentelemetry-api 1.0.0, and #3044 to track doing better than that with muzzle

(also, this is old doc, I'm removing it, see #4106 (comment))

iNikem · 2021-09-29T07:33:41Z

javaagent/build.gradle.kts

@@ -164,17 +184,15 @@ tasks {
    }
  }

-  // Includes everything needed for OOTB experience
-  val fullJavaagentJar by registering(ShadowJar::class) {
+  // Includes instrumentations, but not exporters


Slim does include OTLP exporter, right?

…agent-to-all

trask added 7 commits September 12, 2021 16:49

Change default javaagent to all, introduce slim

af7a83e

Update release builds

d99849d

Update examples build

9c6f04f

Update benchmark-overhead

9b20ff0

Update benchmark-e2e

6ea1a07

Update docs where ok pre-release

6ab2c68

Update misc

6e53ea0

trask requested review from anuraaga, iNikem, jkwatson, laurit, mateuszrzeszutek, pavolloffay and tylerbenson as code owners September 13, 2021 00:27

Fix references to fullJavaagentJar

2a4839c

anuraaga reviewed Sep 13, 2021

View reviewed changes

trask added 7 commits September 12, 2021 20:07

Fix it

64df703

Delete doc for pre-1.0 releases

9c43849

Attach backwards compatible "all" artifact to releases

5113576

Comment

42f4830

Comment

1687020

guessing

ae7e2d1

fix

2b9de15

trask mentioned this pull request Sep 13, 2021

Post 1.7.0 documentation updates #4108

Merged

trask commented Sep 13, 2021

View reviewed changes

anuraaga approved these changes Sep 13, 2021

View reviewed changes

mateuszrzeszutek approved these changes Sep 13, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into change-default-java…

cc35263

…agent-to-all

trask marked this pull request as draft September 14, 2021 05:43

trask added 2 commits September 22, 2021 21:47

Merge remote-tracking branch 'upstream/main' into change-default-java…

6cdfbce

…agent-to-all

Update version

f01fbf6

Merge remote-tracking branch 'upstream/main' into change-default-java…

e6e4e3d

…agent-to-all

trask added 3 commits September 28, 2021 09:59

Merge remote-tracking branch 'upstream/main' into change-default-java…

7b4b731

…agent-to-all

Add otlp/grpc exporter to slim artifact

73226a0

Merge remote-tracking branch 'upstream/main' into change-default-java…

f6c3a94

…agent-to-all

anuraaga reviewed Sep 29, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into change-default-java…

1fa5ef6

…agent-to-all

trask marked this pull request as ready for review September 29, 2021 05:38

iNikem reviewed Sep 29, 2021

View reviewed changes

trask added 2 commits September 29, 2021 09:48

Merge remote-tracking branch 'upstream/main' into change-default-java…

49e1aa8

…agent-to-all

Fix comment

3db6c45

trask mentioned this pull request Sep 29, 2021

Is it safe to exclude the okhttp dependency? #4240

Closed

iNikem approved these changes Sep 30, 2021

View reviewed changes

mateuszrzeszutek approved these changes Sep 30, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into change-default-java…

61ab8fd

…agent-to-all

trask merged commit cfdc4ac into open-telemetry:main Oct 2, 2021

trask deleted the change-default-javaagent-to-all branch October 2, 2021 18:28

bdarfler mentioned this pull request Oct 4, 2021

React to java agent name change from upstream OTel honeycombio/honeycomb-opentelemetry-java#150

Closed

trask mentioned this pull request Sep 26, 2024

opentelemetry-javaagent-all.jar is missing from releases #12338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the default javaagent artifact to have exporters #4106

Change the default javaagent artifact to have exporters #4106

trask commented Sep 13, 2021 •

edited

Loading

anuraaga Sep 13, 2021

trask Sep 13, 2021

trask Sep 13, 2021

breedx-splk Sep 13, 2021

trask Sep 13, 2021

iNikem commented Sep 13, 2021

trask commented Sep 13, 2021

trask commented Sep 14, 2021

trask commented Sep 23, 2021

trask commented Sep 23, 2021

anuraaga Sep 29, 2021

anuraaga Sep 29, 2021

anuraaga Sep 29, 2021

anuraaga Sep 29, 2021

trask Sep 29, 2021

anuraaga Sep 29, 2021

trask Sep 29, 2021

iNikem Sep 29, 2021

trask Sep 29, 2021

iNikem Sep 29, 2021

trask Sep 29, 2021

		// TODO (trask) this needs to be updated now that no more "all" artifact
		return "all".equals(classifier) && "jar".equals(extension);

Change the default javaagent artifact to have exporters #4106

Change the default javaagent artifact to have exporters #4106

Conversation

trask commented Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Sep 13, 2021

trask commented Sep 13, 2021

trask commented Sep 14, 2021

trask commented Sep 23, 2021

trask commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Sep 13, 2021 •

edited

Loading