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

Container detector module kubernetes properties (pod.uid) #1489

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

rahulmukherjee68
Copy link
Contributor

@rahulmukherjee68 rahulmukherjee68 commented Dec 14, 2022

Container detector module kubernetes properties

With this module customer can independently detect the k8s properties like pod.id as a part of span resource attributes and further can be used to correlate any problem with infra instrumentation

Fixes #1474

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • tox -e test-resource-detector-kubernetes

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rahulmukherjee68 rahulmukherjee68 requested a review from a team December 14, 2022 07:00
@rahulmukherjee68 rahulmukherjee68 marked this pull request as draft December 14, 2022 07:01
@rahulmukherjee68 rahulmukherjee68 changed the title Container detector module kubernetes properties (pod.uid Container detector module kubernetes properties (pod.uid) Dec 14, 2022
@rahulmukherjee68 rahulmukherjee68 marked this pull request as ready for review December 19, 2022 14:03
@rahulmukherjee68 rahulmukherjee68 marked this pull request as draft December 19, 2022 14:07
@rahulmukherjee68 rahulmukherjee68 marked this pull request as ready for review December 19, 2022 14:13
Copy link
Member

@sanketmehta28 sanketmehta28 left a comment

Choose a reason for hiding this comment

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

Add entries to eastdist.ini file as well

try:
pod_resource = Resource.get_empty()
try:
pod_uid = (
Copy link
Member

Choose a reason for hiding this comment

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

  • this line will give pod_uid as a tuple which we dont want. you should get pod_uid as a string
  • Also get_kubenertes_pod_uid_v1() and get_kubenertes_pod_uid_v2() is also not required. If we get the pod_id from the v1() then there is no need to get it from v2(). so there should be or operator instead of and

Copy link
Contributor Author

@rahulmukherjee68 rahulmukherjee68 Jan 16, 2023

Choose a reason for hiding this comment

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

The tuple is used since it is not acceptable by lint with out the braces and for the second point i have changed the logic in next commits

def test_k8_id_as_span_attribute_with_cgroup_v2(
self, mock_get_kubenertes_pod_uid_v2
):
tracer_provider, exporter = self.create_tracer_provider(
Copy link
Member

Choose a reason for hiding this comment

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

you dont need to create span for this test case.

"opentelemetry.resource.detector.kubernetes.get_kubenertes_pod_uid_v1",
return_value=f"{MockKubernetesResourceAttributes[ResourceAttributes.K8S_POD_UID]}",
)
def test_k8_id_as_span_attribute_with_mountinfo_v1(
Copy link
Member

Choose a reason for hiding this comment

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

you dont need a span creation for this test case

@mmanciop
Copy link
Contributor

I think this detector could have false-positives on container runtimes outside of Kubernetes. I would add a preliminary check whether the /etc/hosts file is managed by Kubernetes (see kubernetes/kubernetes@fd72938), which is the only (mostly) reliable way I know to check whether a container runs on Kubernetes or not.

@rahulmukherjee68
Copy link
Contributor Author

Thanks for the info @mmanciop . Will be doing that in next commits.

@rahulmukherjee68 rahulmukherjee68 marked this pull request as draft January 16, 2023 12:52
@mmanciop
Copy link
Contributor

If it can help, I have a backport of this PR on lumigo-io/opentelemetry-python-distro#226 (we are waiting to upgrade to a newer Otel SDK version by a bug on the MySQL instrumentation).

@rahulmukherjee68
Copy link
Contributor Author

hey @mmanciop are the following commit was merged in 2018 kubernetes/kubernetes@fd72938
Do you have any idea for the versions before this ? for backward compatibilty

@mmanciop
Copy link
Contributor

mmanciop commented Jan 17, 2023

hey @mmanciop are the following commit was merged in 2018 kubernetes/kubernetes@fd72938 Do you have any idea for the versions before this ? for backward compatibilty

GitHub makes it easy to see in terms of tags:

Screenshot 2023-01-17 at 16 13 02

The change shipped with v1.11. Before v1.11, I don't know of any method.

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.

Resource Detection for Kubernetes properties (e.g. pod.id)
3 participants