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

Nginx autoinstrumentation image #1852

Closed
wants to merge 3 commits into from

Conversation

chrlic
Copy link
Member

@chrlic chrlic commented Jun 18, 2023

This is the first of several PR's to implement autoinstrumentation for Nginx
This one build the image with instrumentation libraries

next will follow

  • Instrumentation crd definitions for Nginx
  • Implementation of instrumentation logic itself + all kinds of tests

@chrlic chrlic requested a review from a team as a code owner June 18, 2023 08:29
############################
# STEP 2 build streamlined image
############################
FROM alpine:latest
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be alpine? Could we change it to from scratch?

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting idea - i think it has to have the busybox at least - let me give it a try.

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to use busybox #1600

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I need a bit more than cp - currently I'm using sh, cp, echo, and sed. Alternatively, I could build all the needed logic in Go, which would have also it's benefits other than using "from scratch". If going this way - where would that go?

Copy link
Member Author

Choose a reason for hiding this comment

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

if I build an image with the instrumentation libraries and a go-based utility which does all the instrumentation logic, which runs without any dependencies in from scratch built image - would that be ok?

RUN mkdir /opt/opentelemetry
WORKDIR /opt/opentelemetry

RUN wget https://github.com/open-telemetry/opentelemetry-cpp-contrib/releases/download/webserver%2Fv$version/opentelemetry-webserver-sdk-x64-linux.tgz
Copy link
Member

Choose a reason for hiding this comment

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

The image seems to be similar to what we already have for apache HTTPd https://github.com/open-telemetry/opentelemetry-operator/blob/main/autoinstrumentation/apache-httpd/Dockerfile#L12

Is there any reason why we should maintain another one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about that, too, but then I thought it would be better to have them separate for versioning reasons. It looks the different versions of the Otel libraries support different versions of Nginx, keeping a common image may bring unwanted dependency in the future.

Copy link
Member

Choose a reason for hiding this comment

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

The version of each instrumentation image is still specified in versions.txt (the default one), via a flag on the operator and in the CR for each instrumentation.

If we don't foresee packaging different libraries in the docker image it does not make sense to create two identical images.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I'm going to use the current one then and if anything prevents using the same image in the future, then I'll handle it accordingly.

I'm going to close this PR and update #1853 with a comment that the apache image is used for Nginx, too.

@chrlic chrlic closed this Jun 23, 2023
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.

None yet

2 participants