-
Notifications
You must be signed in to change notification settings - Fork 454
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
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
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]>
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.
endpoint: http://localhost:4317 | ||
defaults: | ||
useLabelsForResourceAttributes: true |
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.
Please add a new test with this flag set, I'd like to preserve the test that uses the existing 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.
done @TylerHelmuth
@zeitlinger thank you for your contribution! 🙇 |
Fixes #3112
Testing: Added integration test
Documentation: will be added to https://opentelemetry.io/docs/kubernetes/operator/automatic/ once released