Support .NET auto instrumentation and Windows platforms #193

lisguo · 2024-07-11T19:04:48Z

Issue #, if available: N/A

Description of changes:

Add support for .NET auto instrumentation with the current placeholder image public.ecr.aws/aws-observability/adot-autoinstrumentation-dotnet:v0.0.0.
Check if the pod is a windows pod by looking at the node affinity, and if it is windows:
- modify the command to be windows compatible
- use the headless service, as the cluster IP is not supported on windows nodes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…tion image

pkg/instrumentation/auto/config.go

lisguo · 2024-07-12T17:26:45Z

pkg/instrumentation/defaultinstrumentation.go

+
+	cloudwatchAgentServiceEndpoint := "cloudwatch-agent.amazon-cloudwatch"
+	if isWindowsPod {
+		cloudwatchAgentServiceEndpoint = "cloudwatch-agent-windows-headless.amazon-cloudwatch.svc.cluster.local"


we need to use headless endpoint for windows nodes

nitpick - worth adding a comment on why does the service endpoint differ for windows

lisguo · 2024-07-12T17:28:13Z

pkg/instrumentation/defaultinstrumentation_test.go

 			args: args{
 				agentConfig: &adapters.CwaConfig{
 					Logs: &adapters.Logs{
 						LogMetricsCollected: &adapters.LogMetricsCollected{
-							AppSignals: &adapters.AppSignals{},


AppSignals is not used anymore, it's safe to remove. I can have a follow-up PR to remove

lisguo · 2024-07-12T17:29:59Z

pkg/instrumentation/podmutator_test.go


-	defaultInst := &v1alpha1.Instrumentation{


We don't need to test the Instrumentation object here, since it's covered in the getDefaultInstrumentation unit tests

lisguo · 2024-07-12T17:30:39Z

pkg/instrumentation/defaultinstrumentation_test.go

@@ -68,6 +69,23 @@ func Test_getDefaultInstrumentation(t *testing.T) {
 					{Name: "OTEL_LOGS_EXPORTER", Value: "none"},
 				},
 			},
+			DotNet: v1alpha1.DotNet{


@vastin can you confirm these env variables?

It is correct.

lisguo · 2024-07-12T17:30:53Z

pkg/instrumentation/dotnet.go

 	dotnetVolumeName                    = volumeName + "-dotnet"
 	dotnetInitContainerName             = initContainerName + "-dotnet"
 	dotnetInstrMountPath                = "/otel-auto-instrumentation-dotnet"
 )

+const (


@vastin can you confirm these windows mount points too?

Yes. It is confirmed.

Paramadon

LGTM, just want to verify the auto annotation question and if the env vars are correct.

vastin

LGTM

vastin · 2024-07-12T20:22:58Z

pkg/instrumentation/defaultinstrumentation_test.go

@@ -68,6 +69,23 @@ func Test_getDefaultInstrumentation(t *testing.T) {
 					{Name: "OTEL_LOGS_EXPORTER", Value: "none"},
 				},
 			},
+			DotNet: v1alpha1.DotNet{


It is correct.

vastin · 2024-07-12T20:27:29Z

pkg/instrumentation/dotnet.go

 	dotnetVolumeName                    = volumeName + "-dotnet"
 	dotnetInitContainerName             = initContainerName + "-dotnet"
 	dotnetInstrMountPath                = "/otel-auto-instrumentation-dotnet"
 )

+const (


Yes. It is confirmed.

vastin · 2024-07-12T20:33:17Z

pkg/instrumentation/dotnet.go

+	dotNetStartupHookPathWindows     = "C:\\otel-auto-instrumentation-dotnet\\net\\OpenTelemetry.AutoInstrumentation.StartupHook.dll"
+	dotnetInstrMountPathWindows      = "\\otel-auto-instrumentation-dotnet"
+)
+
 // Supported .NET runtime identifiers (https://learn.microsoft.com/en-us/dotnet/core/rid-catalog), can be set by instrumentation.opentelemetry.io/inject-dotnet.
 const (
 	dotNetRuntimeLinuxGlibc = "linux-x64"
 	dotNetRuntimeLinuxMusl  = "linux-musl-x64"


Will CORECLR_PROFILER_PATH be set for "linux-musl-x64" like OTel Operator?

- name: CORECLR_PROFILER_PATH value: /otel-auto-instrumentation-dotnet/linux-musl-x64/OpenTelemetry.AutoInstrumentation.Native.so

pkg/instrumentation/auto/config.go

Paramadon

Looks good!

mitali-salvi

Might be out of scope of this PR but we should be updated the README when providing support for new languages

pkg/instrumentation/auto/config.go

mitali-salvi · 2024-07-16T16:02:51Z

pkg/instrumentation/defaultinstrumentation.go

+
+	cloudwatchAgentServiceEndpoint := "cloudwatch-agent.amazon-cloudwatch"
+	if isWindowsPod {
+		cloudwatchAgentServiceEndpoint = "cloudwatch-agent-windows-headless.amazon-cloudwatch.svc.cluster.local"


nitpick - worth adding a comment on why does the service endpoint differ for windows

…language support

mitali-salvi and others added 11 commits May 8, 2024 15:51

Add support for dotnet instrumentation (aws#168)

2969467

Merge with main

59115cc

Test

bc0157d

Check if the pod is on windows and append -windows to the instrumenta…

191e50f

…tion image

Refactor default instrumentation logic to use windows service

69c7e92

Fix command for .net

251843f

Fix variable name conflict

d8309d7

Change env variables for .NET on windows

8ed7a75

Add OTEL_DOTNET_AUTO_PLUGINS env variable

694a1af

Use headless service for windows

0cf563a

Remove windows specific tags. Use v0.0.0 of adot .net image

91cc6a7

lisguo requested review from movence and Paramadon July 11, 2024 19:04

lisguo added 2 commits July 12, 2024 13:13

Fix unit tests. Run linter

9faca1f

Add OTEL_EXPORTER_OTLP_ENDPOINT env variable for .NET

cdf5ae3

lisguo commented Jul 12, 2024

View reviewed changes

lisguo requested review from sky333999 and removed request for movence July 12, 2024 17:32

Paramadon reviewed Jul 12, 2024

View reviewed changes

Paramadon approved these changes Jul 12, 2024

View reviewed changes

vastin approved these changes Jul 12, 2024

View reviewed changes

Paramadon requested changes Jul 15, 2024

View reviewed changes

pkg/instrumentation/auto/config.go Show resolved Hide resolved

Add DotNet to annotation mutator

c934633

lisguo requested review from mitali-salvi and removed request for sky333999 July 16, 2024 14:42

Paramadon previously approved these changes Jul 16, 2024

View reviewed changes

mitali-salvi reviewed Jul 16, 2024

View reviewed changes

Add comment on windows headless service endpoint. Update README with …

92bc7fa

…language support

lisguo dismissed Paramadon’s stale review via 92bc7fa July 16, 2024 16:16

Paramadon approved these changes Jul 16, 2024

View reviewed changes

mitali-salvi approved these changes Jul 16, 2024

View reviewed changes

lisguo merged commit 5a9b93a into aws:main Jul 16, 2024
6 checks passed

lisguo mentioned this pull request Jul 16, 2024

Add .NET auto instrumentation aws-observability/helm-charts#64

Merged

Paramadon added a commit that referenced this pull request Aug 29, 2024

merging from main (#193) (#217)

9eb6223

Support .NET auto instrumentation and Windows platforms #193

Support .NET auto instrumentation and Windows platforms #193

Uh oh!

Conversation

lisguo commented Jul 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paramadon left a comment

Choose a reason for hiding this comment

Uh oh!

vastin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Paramadon left a comment

Choose a reason for hiding this comment

Uh oh!

mitali-salvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!