-
Notifications
You must be signed in to change notification settings - Fork 483
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
Conversation
############################ | ||
# STEP 2 build streamlined image | ||
############################ | ||
FROM alpine:latest |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This is the first of several PR's to implement autoinstrumentation for Nginx
This one build the image with instrumentation libraries
next will follow