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 Ability to Override Service Names #939

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

Conversation

blakeromano
Copy link

@blakeromano blakeromano commented Jun 21, 2022

Resolves #938

@blakeromano blakeromano requested a review from a team June 21, 2022 21:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: blakeromano / name: Blake R (d12243b)

@mat-rumian
Copy link
Contributor

I think some documentation update would help to make this annotation visible to the community :)

Signed-off-by: blakeromano-il <[email protected]>
@blakeromano
Copy link
Author

I just added some documentation in the readme about this 👍 @mat-rumian

@blakeromano
Copy link
Author

Is there anything else that is needed to get this approved? @mat-rumian

@mat-rumian
Copy link
Contributor

I think @pavolloffay is the right person to ask :)

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

The feature makes sens, but I would like to see a e2e test for this capability

pkg/instrumentation/sdk.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/instrumentation/sdk.go Show resolved Hide resolved
@blakeromano
Copy link
Author

@pavolloffay I made the changes you suggested. I am not super familiar with kuttl but if you feel like e2e tests are needed for this I can do my best to write one up.

README.md Outdated Show resolved Hide resolved
@@ -208,6 +208,10 @@ func (i *sdkInjector) injectCommonSDKConfig(ctx context.Context, otelinst v1alph
}

func chooseServiceName(pod corev1.Pod, resources map[string]string, index int) string {
serviceNames := strings.Split(pod.Annotations["instrumentation.opentelemetry.io/service-name"], ",")
Copy link
Member

Choose a reason for hiding this comment

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

extract the annotation to a constant

Copy link
Member

Choose a reason for hiding this comment

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

Why is it doing the split?

Copy link
Author

Choose a reason for hiding this comment

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

I did the split in the case of people with multiple containers. for you to be able to override the different container names

@pavolloffay
Copy link
Member

@pavolloffay I made the changes you suggested. I am not super familiar with kuttl but if you feel like e2e tests are needed for this I can do my best to write one up.

kuttl is super easy. This functionality deserves e2e tests.

Let me know if you have any question about the e2e tests, I can help.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment on a corner case that I didn't catch before.

metadata:
name: java
spec:
env:
Copy link
Member

Choose a reason for hiding this comment

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

somebody can define OTEL_SERVICE_NAME here. What is the precedence order of setting the service name? We should document this in the readme and have a test.

Copy link
Author

@blakeromano blakeromano Jun 29, 2022

Choose a reason for hiding this comment

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

I honestly am not crazy sure on these answers. If you don't mind helping me fill the gaps @pavolloffay

Copy link
Contributor

Choose a reason for hiding this comment

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

@blakeromano, I'm going to check what is missing to help you out.

Copy link
Author

Choose a reason for hiding this comment

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

@yuriolisa I apologize been busy I was wondering if you had time to help me out 😸

@jaronoff97
Copy link
Contributor

@blakeromano i think this is going to be superseded by #2330 are you interested in continuing this work?

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.

Override Service Name
5 participants