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

Publish a docker image containing Java autoinstrumentation files. #475

Merged
merged 8 commits into from
Nov 3, 2021

Conversation

anuraaga
Copy link
Contributor

To allow auto-instrumentation from the operator, we need to package the actual instrumentation into a docker image for copying into the user's container. This may be a java agent, or a directory of node_modules, or python pip installed packages, etc.

This adds ghcr.io/open-telemetry/autoinstrumentation-java as an image to contain the java instrumentation and is what would be used in conjunction with the pod mutator in https://github.com/open-telemetry/opentelemetry-operator/pull/464/files#diff-8792ce6e230133d6b38ced97a2fdbb6f6282a7c8e5f939cc7783521be7823232R59

Note that I discussed with the Java maintainers, and we didn't find there to be enough value in a base image for end users to consume to publish it there, so instead this is meant to be more of an internal detail of the operator. And it would make even less sense for something like Python, where end users just use pip anyways for example (javaagent users don't have such a package management approach in general making it slightly unique).

Ran the workflow on my fork to produce https://github.com/anuraaga/opentelemetry-operator/pkgs/container/opentelemetry-operator%2Fautoinstrumentation-java

@anuraaga anuraaga requested review from a team and owais October 27, 2021 07:41
@@ -0,0 +1 @@
1.7.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For other languages, this will be a version number in something like a package.json. I tried to provide a similar "edit a file to update the version" experience for Java using this txt file.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative approach would be to put the version in https://github.com/open-telemetry/opentelemetry-operator/blob/main/versions.txt and publish the image on operator release.

However, I like your approach more as it allows us to release the java docker image without operator release. I still need the version being defined in the root version.txt to use it as a default version of the javaagent when the image is not specified in the CR (I will do this on a follow up PR once the initial implementation is merged).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - yeah since instrumentation will be published at various cycles it's probably good to decouple those release cycles.

Added the version to top level to allow operator to reference the most recent one (which will generally have already been published before the operator is released)

@anuraaga
Copy link
Contributor Author

/cc @pavolloffay

@@ -0,0 +1 @@
1.7.0
Copy link
Member

Choose a reason for hiding this comment

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

An alternative approach would be to put the version in https://github.com/open-telemetry/opentelemetry-operator/blob/main/versions.txt and publish the image on operator release.

However, I like your approach more as it allows us to release the java docker image without operator release. I still need the version being defined in the root version.txt to use it as a default version of the javaagent when the image is not specified in the CR (I will do this on a follow up PR once the initial implementation is merged).

@pavolloffay
Copy link
Member

@VineethReddy02 @jpkrohling could you please take a look and move this forward?

.github/workflows/publish-autoinstrumentation-java.yaml Outdated Show resolved Hide resolved
autoinstrumentation/java/Dockerfile Show resolved Hide resolved
versions.txt Show resolved Hide resolved
Copy link
Contributor

@VineethReddy02 VineethReddy02 left a comment

Choose a reason for hiding this comment

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

LGTM! I think after iterating over Juraci comments. We can go ahead with the merge :)

@pavolloffay
Copy link
Member

@anuraaga the CI failed

@jpkrohling
Copy link
Member

@VineethReddy02, can you look at the failure? Looks like it's not related to this PR.

@pavolloffay
Copy link
Member

This is the CI failure

=== RUN   Test0_36_0Upgrade
    v0_36_0_test.go:119: 
        	Error Trace:	v0_36_0_test.go:119
        	Error:      	Not equal: 
        	            	expected: "upgrade to v0.36.0 has changed the tls_settings field name to tls in grpc protocol of otlp/mtls receiver"
        	            	actual  : "upgrade to v0.36.0 has changed the tls_settings field name to tls in http protocol of otlp/mtls receiver"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-upgrade to v0.36.0 has changed the tls_settings field name to tls in grpc protocol of otlp/mtls receiver
        	            	+upgrade to v0.36.0 has changed the tls_settings field name to tls in http protocol of otlp/mtls receiver
        	Test:       	Test0_36_0Upgrade
    v0_36_0_test.go:120: 
        	Error Trace:	v0_36_0_test.go:120
        	Error:      	Not equal: 
        	            	expected: "upgrade to v0.36.0 has changed the tls_settings field name to tls in http protocol of otlp/mtls receiver"
        	            	actual  : "upgrade to v0.36.0 has changed the tls_settings field name to tls in grpc protocol of otlp/mtls receiver"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-upgrade to v0.36.0 has changed the tls_settings field name to tls in http protocol of otlp/mtls receiver
        	            	+upgrade to v0.36.0 has changed the tls_settings field name to tls in grpc protocol of otlp/mtls receiver
        	Test:       	Test0_36_0Upgrade
--- FAIL: Test0_36_0Upgrade (0.00s)

Indeed not related to this PR.

@pavolloffay
Copy link
Member

Here is fix #494

@jpkrohling
Copy link
Member

This PR needs a rebase, to get the fix for the flaky test.

@pavolloffay
Copy link
Member

@jpkrohling CI is green now

@jpkrohling jpkrohling merged commit 9e16767 into open-telemetry:main Nov 3, 2021
shree007 pushed a commit to shree007/opentelemetry-operator that referenced this pull request Dec 12, 2021
…en-telemetry#475)

* Publish a docker image containing Java autoinstrumentation files.

* Push to anuraaga temporarily

* Push to open-telemetry

* Allow yaml files for autoinstrumentation to be grouped together

* Add reference to java instrumentation version for use in operator

* Quay
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…en-telemetry#475)

* Publish a docker image containing Java autoinstrumentation files.

* Push to anuraaga temporarily

* Push to open-telemetry

* Allow yaml files for autoinstrumentation to be grouped together

* Add reference to java instrumentation version for use in operator

* Quay
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.

4 participants