-
Notifications
You must be signed in to change notification settings - Fork 24
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 support for python instrumentation #69
Conversation
@@ -47,6 +47,9 @@ manager: | |||
java: | |||
repository: public.ecr.aws/aws-observability/adot-autoinstrumentation-java | |||
tag: v1.31.1 | |||
python: | |||
repository: ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-python |
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.
Is there a reason we're using GHCR instead of ECR? Is it not available yet?
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.
Yes this is temporary until the ADOT python image is available.
main.go
Outdated
autoInstrumentationPython string | ||
autoAnnotationConfigStr string | ||
webhookPort int |
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.
nit: Is this just a weird formatting issue? Did you run make fmt
?
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.
this is what i get for resolving merge conflicts from the github ui sigh. I knew I should've used cli
defaultAPIVersion = "cloudwatch.aws.amazon.com/v1alpha1" | ||
defaultInstrumenation = "java-instrumentation" | ||
defaultNamespace = "default" | ||
defaultKind = "Instrumentation" |
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 have any code change? Or we are just reformatting the code?
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.
Just reformatting. I decided to group different attributes by different use cases
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.
What does default instrumentation and config now mean when we have multiple instrumentations? I don't think OTel operator has a default instrumentation. It will inject whatever instrumentation is defined via the annotation.
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 Instrumentation CR defines multiple languages. The default instrumentation will mutate pods even if the customer has no Instrumentation CR on their cluster.
This file has the definition of what the environment variables we will inject when pods are annotated.
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 default instrumentation will mutate pods even if the customer has no Instrumentation CR on their cluster.
Still not clear why this is needed to be done.
@@ -32,13 +33,23 @@ const ( | |||
otelExporterSmpEndpointDefaultValue = "http://cloudwatch-agent.amazon-cloudwatch:4315" |
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 we use OTEL_EXPORTER_OTLP_METRICS_PROTOCOL: http/protobuf
, do we need metrics endpoint to be something like: http://cloudwatch-agent.amazon-cloudwatch:4316/v1/metrics
?
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 am not sure, I left these variables to be the same as java for now at least to be backwards compatible.
We can always revisit this variables 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.
We only care about the OTEL_AWS_SMP_EXPORTER_ENDPOINT
for appsignals metrics. If the endpoint says port 4315
(which it will for EKS) then the SMP exporter will be a grpc exporter.
@@ -32,13 +33,23 @@ const ( | |||
otelExporterSmpEndpointDefaultValue = "http://cloudwatch-agent.amazon-cloudwatch:4315" | |||
otelExporterMetricKey = "OTEL_METRICS_EXPORTER" | |||
otelExporterMetricDefaultValue = "none" | |||
|
|||
otelPythonDistro = "OTEL_PYTHON_DISTRO" | |||
otelPythonDistroDefaultValue = "aws-distro" |
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.
Should be "aws_distro"
otelPythonDistro = "OTEL_PYTHON_DISTRO" | ||
otelPythonDistroDefaultValue = "aws-distro" | ||
otelPythonConfigurator = "OTEL_PYTHON_CONFIGURATOR" | ||
otelPythonConfiguratorDefaultValue = "aws-configurator" |
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.
Should be "aws_configurator"
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.
Mostly went through the configuration stuff and LGTM, except aws_distro
and aws_configurator
changes. TY
defaultNamespace = "default" | ||
defaultKind = "Instrumentation" | ||
defaultAPIVersion = "cloudwatch.aws.amazon.com/v1alpha1" | ||
defaultInstrumenation = "java-instrumentation" |
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.
This is going to look very weird. A follow up to unravel the implications of changing this for existing customers would be good so we atleast understand.
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.
Actually we never actually create this resource on the cluster.. its just referenced in memory.
So it might be ok to rename this.
We need to test out the scenario to be sure... but this is non-blocking.
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.
you mean the naming of the instrumentation cr itself? yeah because this is no longer just java.
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.
yup thats right since this is no longer just java and currently we call it java-instrumentation
. No one will ever see this since its not actually created on the cluster and only referenced below by the operator.. so not a blocker.
defaultInstrumenation = "java-instrumentation" | ||
defaultNamespace = "default" | ||
defaultKind = "Instrumentation" | ||
defaultAPIVersion = "cloudwatch.aws.amazon.com/v1alpha1" |
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 personally find it hard to navigate these consts back & forth between the usages below and the values here when trying to see what the instrumentation CR actually looks like.
If these consts are not used in more than one place, I find inlining them to be more readable.. but might just be me.
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.
yeah I was debating on that too -- the only thing is that we do re-use the same env vars for each lang..maybe having an env map would be better
Description of changes:
ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-python:0.43b0
instrumentation.opentelemetry.io/inject-python: true
:By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.