open-telemetry / opentelemetry-cpp-contrib

https://opentelemetry.io/
Apache License 2.0
123 stars 137 forks source link

[BUILD] otel-webserver add support for arm64 #400

Open Fydon opened 6 months ago

Fydon commented 6 months ago

Fixes #343.

Attempt to enable otel-webserver support for ARM64. The build succeeds and I appear to able to use the webserver module on ARM64.

  1. Should the Dockerfiles, e.g. instrumentation\otel-webserver-module\docker\ubuntu20.04\Dockerfile be updated to have the platforms in different paths?
  2. Should the ARM64 build use the gccArchFlag of -march=armv8-a or should it be one of the others.
linux-foundation-easycla[bot] commented 6 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

Fydon commented 6 months ago

The failure for centos7 is ERROR: write /root/.gradle/wrapper/dists/gradle-4.10.2-all/9fahxiiecdb76a5g3aw9oi8rv/gradle-4.10.2-all.zip: no space left on device

aryanishan1001 commented 6 months ago

Hey, can you pull the code again and push your changes. Some builds were failing, so changes have been made to the build pipeline.

Fydon commented 6 months ago

Would someone be able to run to pipeline to see if it now works? Also please let me know if I should make any changes, e.g. in relation to the questions in my original post.

Fydon commented 6 months ago

Sorry I see that docker compose v1 (docker-compose) was in use. Updated to docker compose v2 (docker compose), which I think resolves the error. Also it probably required QEMU to perform ARM64 builds.

Fydon commented 5 months ago

Thank you for running the workflow. The current GitHub workflow is only building the x64 artifacts. Should I duplicate the jobs so that there is a build for each platform and operating system, i.e. webserver-build-test-ubuntu-x64, webserver-build-test-ubuntu-arm64, webserver-build-test-centos7-x64 and webserver-build-test-centos7-arm64, or should the build step change to build both in a single build step, e.g. docker-compose --profile ubuntu20.04 build? With the single build step, there would probably need to still be separate unit test, upload artifacts and run integrationtest steps for each platform.

marcalff commented 5 months ago

Thank you for running the workflow. The current GitHub workflow is only building the x64 artifacts. Should I duplicate the jobs so that there is a build for each platform and operating system, i.e. webserver-build-test-ubuntu-x64, webserver-build-test-ubuntu-arm64, webserver-build-test-centos7-x64 and webserver-build-test-centos7-arm64, or should the build step change to build both in a single build step, e.g. docker-compose --profile ubuntu20.04 build? With the single build step, there would probably need to still be separate unit test, upload artifacts and run integrationtest steps for each platform.

No idea, this is a question for the otel-webserver maintainers, who know this area best.

I just press the workflow button ;-)

Fydon commented 5 months ago

Thank you @marcalff. I assume that the maintainers would also be reviewing these changes at some point and so be able to advise?

I also noticed that the unit tests are currently platform specific, so I'm working on updating gradle to handle this.

Fydon commented 5 months ago

I've gone for having extra jobs for ARM64. The unit tests now work for both platforms, but the integrations tests don't work on my Windows PC for x64 or ARM64. They may work in the GitHub Action.

When I run CentOS 7 with ARM64 locally, I run into a problem with QEMU and yum so instead I attempted to have the Ubuntu job build the artifacts. Given that there are difference between the CentOS and Ubuntu Dockerfiles, is there a problem with using Ubuntu for releases?

Fydon commented 5 months ago

I tried using podman 5 instead of docker to build the CentOS 7 ARM release and the build succeeded. Unfortunately it got stuck exporting the layers of the image and so couldn't progress beyond that. However as the integration test failed in the GitHub action with the Ubuntu build, I thought it worth seeing if the GitHub action would get beyond the first yum command (4th command in Dockerfile), unlike my experience with Docker Desktop 4.29.0 (145265).

deepaksrivastavaz commented 5 months ago

Hi @Fydon , I am assuming this is supposed to work on below arch as well, can you please confirm. 5.10.199-190.747.amzn2.aarch64

We thought of instrumenting nginx with otel but it failed there.

Fydon commented 5 months ago

I have only used the apache functionality. Yes aarch64 is ARM64. This is a step to having this work, but it may require more work. Particularly the integration tests may need to be changed if they aren't testing ARM64. If using a clone of the latest version of this pull request is not working for you feel free to make the necessary changes and contribute them once this is merged.

Fydon commented 5 months ago

GitHub has a beta for ARM based runners for GitHub Actions. That should significantly improve the performance of these builds. Perhaps for now the solution is to review the changes, but not run the CentOS ARM64 build?

I'm using the Apache ARM64 build on Ubuntu 22.04 to obtain traces, but have not tested Nginx.

Fydon commented 5 months ago

Okay. I think that is done

Fydon commented 5 months ago

Sorry I realised that I left the build arguments in the CentOS 7 ARM build. I've committed but not pushed removing these as it will cause another need for a build. There will probably be more changes as a part of the review so they will be a part of the next update.

Fydon commented 3 months ago

ARM64 hosted runners have now been released. Could you please set one up so that the workflow can run quicker? If so, please let me know the name so that I can update the workflow.

pl4nty commented 1 month ago

there are x64 references in otel-webserver-module/src/nginx/script.sh and Makefile that caused an undefined symbol: matchIgnorePathRegex fatal error with my similar patchset, and might need to be addressed here

Fydon commented 1 month ago

@pl4nty I don't currently use nginx. If you can provide the necessary changes, I can add them. I've use the ARM64 build with Apache so that does work.

However this pull request probably needs a maintainer to add a ARM64 host runner so that the build can complete.

pl4nty commented 1 month ago

yeah I've been running httpd too, only tested nginx yesterday. not sure if there's a way to do this with variables

--- a/instrumentation/otel-webserver-module/src/nginx/Makefile
+++ a/instrumentation/otel-webserver-module/src/nginx/Makefile
@@ -350,6 +350,7 @@ objs/nginx: objs/src/core/nginx.o \
        objs/src/http/modules/ngx_http_upstream_zone_module.o \
        objs/ngx_modules.o \
        -L/root/webserver-agent/build/linux-x64/opentelemetry-webserver-sdk/sdk_lib/lib -lopentelemetry_webserver_sdk -ldl -lrt -lpthread -lcrypt -lpcre -lz \
+       -L/root/webserver-agent/build/linux-arm64/opentelemetry-webserver-sdk/sdk_lib/lib -lopentelemetry_webserver_sdk -ldl -lrt -lpthread -lcrypt -lpcre -lz \
        -Wl,-E

@@ -1207,6 +1208,8 @@ objs/ngx_http_opentelemetry_module.so:    objs/addon/nginx/ngx_http_opentelemetry_l
        objs/addon/nginx/ngx_http_opentelemetry_module.o \
        objs/ngx_http_opentelemetry_module_modules.o \
        -L../linux-x64/opentelemetry-webserver-sdk/sdk_lib/lib \
+    -lopentelemetry_webserver_sdk \
+       -L../linux-arm64/opentelemetry-webserver-sdk/sdk_lib/lib \
     -lopentelemetry_webserver_sdk \
        -shared
--- a/instrumentation/otel-webserver-module/src/nginx/script.sh
+++ b/instrumentation/otel-webserver-module/src/nginx/script.sh
@@ -2,4 +2,6 @@
 fileName=$1

 sed -i "s/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lrt\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/g" $fileName
+sed -i "s/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lrt\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/g" $fileName
 sed -i "s/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ \\\/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ \\\/g" $fileName
+sed -i "s/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ \\\/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ \\\/g" $fileName