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

Add arm64 artefacts to dotnet autoinstrumentation container. #3270

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Mpdreamz
Copy link

@Mpdreamz Mpdreamz commented Sep 6, 2024

This will allow the operator to auto instrument dotnet applications on arm64 containers. (Including k8s on OSX M1/M2/M3 etc)

Validated manually as part of Elastic's distribution of OpenTelemetry .NET

elastic/elastic-otel-dotnet#155

CC @Kielek & @RassK who did the bulk of the work actually creating the artefacts 🙏

This will allow the the operator to instrument `arm64` containers. (Including k8s on OSX M1/M2/M3 etc)
@Mpdreamz Mpdreamz requested a review from a team September 6, 2024 19:41
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

  1. As I know, there is a possibility to have separate images for architectures (arm64/x64). It could reduce image size to download. I am not telling that it is mandatory for this PR.
  2. It will not work without additional changes on the operator side. See code around
    const (
    dotNetRuntimeLinuxGlibc = "linux-x64"
    dotNetRuntimeLinuxMusl = "linux-musl-x64"
    )

Copy link
Contributor

Choose a reason for hiding this comment

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

Please document changes around lines 7 and 8 - CORECLR_PROFILER_PATH

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

It doesn't look like this fully addresses the issue. The operator will still set the CORECLR_PROFILER_PATH to the x64 path.

I think a clean way to implement this would be:

  1. Make the image multi-platform, changing the path to just linux. Add a linux-x64 symbolic link for backwards compatibility.
  2. Use the new path when injecting instrumentation.

For 1, you just need to add the built-in ARG TARGETARCH and use this variable in your urls.

@Mpdreamz
Copy link
Author

Just a note that I will get back to this PR either this or next week.

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