[processor/resourcedetection] Detect Azure Kubernetes Service cluster name #29328

jsirianni · 2023-11-17T14:28:53Z

Description:

Added best effort support for detecting Azure Kubernetes Service cluster name: k8s.cluster.name.

The cluster name can be extracted from the cluster's "resource group name" which is retrieved using existing functionality. The parseClusterName function has comments explaining the limitations.

Link to tracking Issue:

#26794

Testing:

I added unit tests for each scenario, and have tested against live AKS clusters that fit each scenario. I am happy to spin these up if anyone has any questions.

Documentation:

Added k8s.cluster.name to the list of AKS resource attributes.

mx-psi · 2023-11-20T13:31:40Z

cc @jinja2 @jsirianni. Would also be great if someone from Microsoft reviews this :)

jinja2 · 2023-11-20T23:27:07Z

processor/resourcedetectionprocessor/internal/azure/aks/metadata.yaml

+    enabled: true
+  k8s.cluster.name:
+    description: The k8s.cluster.name


Can we add some details in the description about how the cluster name is detected? The comments in the code are great but a user won't know to refer to them and might assume that this is set to the cluster resource name which might not be the case.

Thank you for taking a look. I added some clarity to the metadata yaml and readme.

jsirianni · 2023-12-04T19:03:15Z

@mx-psi Let me know if there is anything else I need to do here 👍

jinja2 · 2023-12-04T19:14:02Z

processor/resourcedetectionprocessor/README.md

+
+The cluster name is accurately detected if underscores are not present in the resource group name or the cluster name.
+
+If accurate parsing cannot be performed, the infrastructure resource group value is returned. This value can be used to accurately identify the cluster, as Azure will not allow users to create multiple clusters with the same infrastructure resource group.


Suggested change

If accurate parsing cannot be performed, the infrastructure resource group value is returned. This value can be used to accurately identify the cluster, as Azure will not allow users to create multiple clusters with the same infrastructure resource group.

If accurate parsing cannot be performed, the infrastructure resource group value is returned. This value can be used to uniquely identify the cluster, as Azure will not allow users to create multiple clusters with the same infrastructure resource group name.

jinja2 · 2023-12-04T19:22:38Z

processor/resourcedetectionprocessor/README.md

+
+Azure AKS cluster name is derived from the Azure Instance Metadata Service's (IMDS) infrastructure resource group field. This field contains the resource group and name of the cluster, separated by underscores. e.g: `MC_<resource group>_<cluster name>_<location>`.
+
+The cluster name is accurately detected if underscores are not present in the resource group name or the cluster name.


Imo we should clarify that we can only get the correct cluster name if custom resource group name is not used. I still think some users might prefer setting the clusterName themself then defaulting to custom resource group name. So explicitly making a note of what is returned in different scenarios would provide better user experience.

Agreed. Apologies for the late response.

mx-psi · 2023-12-05T11:17:43Z

@mx-psi Let me know if there is anything else I need to do here 👍

I was waiting on jinja2 to finish their review before I took a look :)

jinja2 · 2023-12-05T16:55:45Z

processor/resourcedetectionprocessor/internal/azure/aks/metadata.yaml

+    description: The k8s.cluster.name parsed from the Azure Instance Metadata Service's infrastructure resource group field
+    type: string
+    enabled: true


Suggested change

enabled: true

enabled: false

I think we should keep it disabled by default.

github-actions · 2024-01-02T05:20:02Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mx-psi · 2024-01-03T10:05:11Z

@jinja2 Mind taking another look?

dmitryax · 2024-01-08T23:44:36Z

processor/resourcedetectionprocessor/internal/azure/aks/aks.go

+// because Azure will not allow the user to create multiple AKS clusters with the same
+// infrastructure resource group name.
+func parseClusterName(resourceGroup string) string {
+	// Code inspired by https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/datadogexporter/internal/hostmetadata/internal/azure/provider.go#L36


Nice description for parseClusterName 👍

Assuming that this code won't be removed from exporter/datadogexporter going forward (cc @mx-psi to confirm), I think we should consolidate it in internal/azure and use it both here and in exporter/datadogexporter. Moving it from exporter/datadogexporter to internal/azure can be done in a separate PR.

We are not going to remove this from the Datadog exporter in the short term,so I agree we should consolidate this 👍

Cool, no action needed from me?

So we'd like to make it a common utility in internal/azure and update the components, but can be done in a separate PR. @dmitryax if this is not a blocker, ~~I think we are good to merge this~~ needs some testing updates

Sounds good to me as a follow-up PR. @jsirianni will you have a chance to submit that PR?

@dmitryax I think I can handle that 👍

@jsirianni thanks. Submitted #30834

jinja2 · 2024-01-17T17:58:08Z

processor/resourcedetectionprocessor/internal/azure/aks/aks_test.go

@@ -34,6 +34,8 @@ func TestDetector_Detect_K8s_Azure(t *testing.T) {
 	assert.Equal(t, map[string]any{
 		"cloud.provider": "azure",
 		"cloud.platform": "azure_aks",
+		// Cluster name will not be detected if resource grup is not set
+		"k8s.cluster.name": "",


If the attribute is not enabled, it won't be available in the output, so you want to remove this. See failing test here.
I think it would be better to add another test case with the attribute enabled.

… name (open-telemetry#29328) **Description:** Added best effort support for detecting Azure Kubernetes Service cluster name: `k8s.cluster.name`. The cluster name can be extracted from the cluster's "resource group name" which is retrieved using existing functionality. The `parseClusterName` function has comments explaining the limitations. **Link to tracking Issue:** open-telemetry#26794 **Testing:** I added unit tests for each scenario, and have tested against live AKS clusters that fit each scenario. I am happy to spin these up if anyone has any questions. Added `k8s.cluster.name` to the list of AKS resource attributes.

jsirianni requested a review from mx-psi as a code owner November 17, 2023 14:28

jsirianni requested a review from a team November 17, 2023 14:28

jsirianni requested review from Aneurysm9 and dashpole as code owners November 17, 2023 14:28

github-actions bot assigned mx-psi Nov 17, 2023

github-actions bot added the processor/resourcedetection Resource detection processor label Nov 17, 2023

jinja2 reviewed Nov 20, 2023

View reviewed changes

jsirianni force-pushed the azure/detect-cluster-name branch from 125db6d to 0065a48 Compare December 4, 2023 19:02

jinja2 reviewed Dec 4, 2023

View reviewed changes

jinja2 reviewed Dec 5, 2023

View reviewed changes

jsirianni force-pushed the azure/detect-cluster-name branch from 0065a48 to 030bdc1 Compare December 18, 2023 15:45

jsirianni requested a review from jinja2 December 18, 2023 15:45

github-actions bot added the Stale label Jan 2, 2024

mx-psi removed the Stale label Jan 3, 2024

dmitryax mentioned this pull request Jan 8, 2024

[processor/resourcedetection]: Add support for k8s.cluster.name where possible #26794

Closed

dmitryax reviewed Jan 8, 2024

View reviewed changes

jsirianni force-pushed the azure/detect-cluster-name branch from 030bdc1 to 197d3d5 Compare January 12, 2024 16:10

jinja2 reviewed Jan 17, 2024

View reviewed changes

jsirianni added 7 commits January 25, 2024 15:57

wip: detect aks cluster name

1b839f9

re-write doc and consider the mc prefix

54db3d9

add tests for happy path and common failures

74a2cc7

remove debug print

b03daa9

add changelog entry

2ea6943

update aks resource attribute list

b08a5f6

unnest k8s.cluster.name

6feb5e0

jsirianni added 5 commits January 25, 2024 15:57

refactor to call imds endpoint once

c7445d0

document aks cluster name parsing

a9be94b

pr feedback: Disable cluster name by default. Misc spelling / wording.

f6e6efb

Re generate

a68f8b0

fix TestDetector_Detect_K8s_Azure

3f0c0e3

jsirianni force-pushed the azure/detect-cluster-name branch from 18236de to 3f0c0e3 Compare January 25, 2024 21:02

dmitryax approved these changes Jan 29, 2024

View reviewed changes

dmitryax merged commit a13a115 into open-telemetry:main Jan 29, 2024
85 checks passed

github-actions bot added this to the next release milestone Jan 29, 2024

dmitryax mentioned this pull request Jan 29, 2024

[internal/azure] Move cluster name discovery mechanism to an internal package #30834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[processor/resourcedetection] Detect Azure Kubernetes Service cluster name #29328

[processor/resourcedetection] Detect Azure Kubernetes Service cluster name #29328

jsirianni commented Nov 17, 2023

mx-psi commented Nov 20, 2023

jinja2 Nov 20, 2023

jsirianni Nov 21, 2023

jsirianni commented Dec 4, 2023

jinja2 Dec 4, 2023

jinja2 Dec 4, 2023

jsirianni Dec 18, 2023

mx-psi commented Dec 5, 2023

jinja2 Dec 5, 2023

github-actions bot commented Jan 2, 2024

mx-psi commented Jan 3, 2024

dmitryax Jan 8, 2024 •

edited

Loading

mx-psi Jan 9, 2024

jsirianni Jan 12, 2024

jinja2 Jan 17, 2024 •

edited

Loading

dmitryax Jan 17, 2024

jsirianni Jan 25, 2024

dmitryax Jan 29, 2024

jinja2 Jan 17, 2024


		The cluster name is accurately detected if underscores are not present in the resource group name or the cluster name.

		If accurate parsing cannot be performed, the infrastructure resource group value is returned. This value can be used to accurately identify the cluster, as Azure will not allow users to create multiple clusters with the same infrastructure resource group.


		Azure AKS cluster name is derived from the Azure Instance Metadata Service's (IMDS) infrastructure resource group field. This field contains the resource group and name of the cluster, separated by underscores. e.g: `MC_<resource group>_<cluster name>_<location>`.

		The cluster name is accurately detected if underscores are not present in the resource group name or the cluster name.

[processor/resourcedetection] Detect Azure Kubernetes Service cluster name #29328

[processor/resourcedetection] Detect Azure Kubernetes Service cluster name #29328

Conversation

jsirianni commented Nov 17, 2023

mx-psi commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirianni commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Dec 5, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2024

mx-psi commented Jan 3, 2024

dmitryax Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinja2 Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitryax Jan 8, 2024 •

edited

Loading

jinja2 Jan 17, 2024 •

edited

Loading