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

Cross namespace instrumentation #889

Merged

Conversation

tKe
Copy link
Contributor

@tKe tKe commented May 24, 2022

This PR adds support for specifying the namespace of an instrumentation instance in the inject-* annotations.

closes #886

@tKe tKe requested a review from a team May 24, 2022 12:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 24, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@ringerc
Copy link

ringerc commented May 27, 2022

This looks sensible as an idea.

Over in #886 (comment) I pointed out how some other services like Istio do it, using an exportTo field in the resource rather than a namespace qualification in the annotation. But either seems fine, and having this functionality would be really nice.

Copy link
Contributor

@VineethReddy02 VineethReddy02 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! @tKe
LGTM!

@pavolloffay can you review this PR from your end? :)

Copy link
Contributor

@yuriolisa yuriolisa left a comment

Choose a reason for hiding this comment

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

LGTM!

@pavolloffay pavolloffay merged commit f351a6d into open-telemetry:main Jun 1, 2022
@@ -118,8 +118,15 @@ func (pm *instPodMutator) getInstrumentationInstance(ctx context.Context, ns cor
return pm.selectInstrumentationInstanceFromNamespace(ctx, ns)
}

var instNamespacedName types.NamespacedName
Copy link
Member

Choose a reason for hiding this comment

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

@tKe given we are changing the pod mutator that is used by the sidecar injection as well then the cross-namespace feature should work for the sidecar injector as well and we should document it. Would you like to submit a separate PR to document it for the sidecar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pod mutator is only used for instrumentation. The sidecar injection uses a separate pod mutator which is not impacted. The sidecar injection cannot currently work across namespaces due to the requirement to mount the collector configmap.

@tKe tKe deleted the cross-namespace-instrumentation branch June 1, 2022 12:19
@ringerc
Copy link

ringerc commented Jun 4, 2022

I love PRs where the README.md update is right there when it is opened.

This is an exciting change to have. I was about to hack up my infrastructure to add a complicated mess of Kustomize Component injections to work around this - and now I don't have to.

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* support cross-namespace instrumentation reference

* update readme
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.

support cross-namespace instrumentation references in annotations
5 participants