-
Notifications
You must be signed in to change notification settings - Fork 414
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
read resource attributes from annotations #3204
base: main
Are you sure you want to change the base?
read resource attributes from annotations #3204
Conversation
per our discussion, @zeitlinger will rebase and expand on the capabilities introduced by #2330 :) |
cfe0591
to
a2f5aa4
Compare
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.
Can you also update the README.md, explaining the changes and also explaining the precedence of deciding these variables?
5edb85e
to
220650b
Compare
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.
@zeitlinger last thing – can you update the README.md with some more information about the new behavior? Maybe also include it in the subtext of the chlog entry
pkg/instrumentation/sdk.go
Outdated
if name := chooseLabelOrAnnotation(pod, semconv.ServiceNameKey, constants.AnnotationAppName); name != "" { | ||
return name | ||
} |
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 concerned that this would be a breaking change.
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, I'll include that in the changelog
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.
@jaronoff97 @pavolloffay I am pretty worried about this breaking change. Changes to service.name
will change emitted telemetry, which could affect end-users alerts/SLOs/dashboards/etc. This change will (likely) not be detected with unit or e2e tests before a user updates their operator. Additionally, the operator's auto-instrumentation injection is already a huge obfuscation on top of already obfuscated auto-instrumentation, which means that our end-users are even less likely to understand the impact of this change.
I would much rather prefer this work be implemented in such a way that the annotation is used as the service name if no other service name is set OR if some other explicit configuration field is set that enables this behavior.
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 reason I call this out as a breaking change in the current implementation is that it is now defaulting to the annotation value if it exists. Since it is a common annotation, it will probably be on most end-user's pods, resulting in the potential breaking change.
I would much rather this be the last option in this list to avoid the breaking change.
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.
In addition, there is no stable semantic convention that ranks which value should be used as the service.name
. I'd prefer avoiding breaking changes on this topic until there is a stable semantic convention, so that we can minimize the number of breaking changes.
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 could solve this problem if we let the user define the mapping they prefer - so it would be an opt-in feature
something like this - should be similar to https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-pod-fields-as-values-for-environment-variables
we can even use this public method (passing in a fieldRef): https://github.com/kubernetes/kubernetes/blob/a9593d634c6a053848413e600dadbf974627515f/staging/src/k8s.io/kubectl/pkg/cmd/set/env/env_resolve.go#L248
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
name: my-instrumentation
spec:
resource:
attributes:
- key: service.name
valueFrom:
fieldRef:
fieldPath: metadata.labels['test.example.com/key']
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.
from SIG:
I think we should move forward with this by putting it within a new "defaults" section in the instrumentation CRD as so:
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
name: my-instrumentation
spec:
defaults:
useLabelsForResource: true
We also must then document in the readme the precedence for the automatic variables we set for users and how this boolean changes that.
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 have useLabelsForResourceAttributes
now
However, I've noticed that it could also make sense here:
AddK8sUIDAttributes bool `json:"addK8sUIDAttributes,omitempty"` |
WDYT?
done |
8745d54
to
baf151c
Compare
CI failed |
20c4bb7
to
a273927
Compare
@jaronoff97 please check again |
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.
another thought, also if we could add something in to an existing e2e test that would be great :) thank you! FWIW, i do think I will be taking on a refactor to make this logic a bit more straightforward in the future.
a7c7322
to
cc6338a
Compare
Co-authored-by: Jacob Aronoff <[email protected]>
Co-authored-by: Jacob Aronoff <[email protected]>
cc6338a
to
2e9f6ec
Compare
done @jaronoff97 |
pkg/instrumentation/sdk.go
Outdated
for k, v := range pod.Annotations { | ||
if strings.HasPrefix(k, constants.ResourceAttributeAnnotationPrefix) { | ||
k = strings.TrimPrefix(k, constants.ResourceAttributeAnnotationPrefix) | ||
if !existingRes[k] && k != string(semconv.ServiceNameKey) { |
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.
don't we want to let the override happen here?
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 treat the user defined enviroment variables as the highest priority - otherwise it would be a breaking change.
Not sure if it would also be surprising.
@jaronoff97 please check again |
@@ -259,7 +529,7 @@ func TestSDKInjection(t *testing.T) { | |||
Env: []corev1.EnvVar{ | |||
{ | |||
Name: "OTEL_SERVICE_NAME", | |||
Value: "explicitly_set", | |||
Value: "explicit-name", |
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.
OOC – why change this for the test?
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.
to make sure it's coming from the right place
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.
two minor questions, neither blocking. overall i think this looks good. Going to wait on @TylerHelmuth's review before merging.
Fixes #3112
Testing: Added integration test
Documentation: will be added to https://opentelemetry.io/docs/kubernetes/operator/automatic/ once released