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.

Sign up for GitHub

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

k8sprocessor: add container.name field #1167

Closed
wants to merge 9 commits into from
Closed

k8sprocessor: add container.name field #1167

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9fc15db
k8sprocessor: add host.name and container.name fields (#188)
pmalek-sumo Oct 1, 2020
2ea31b3
Minor naming and structure changes
pmalek Oct 1, 2020
5a054f5
container.name -> k8s.container.name
pmalek Oct 1, 2020
1f7b843
Keep it singular
pmalek Oct 1, 2020
86dc2cb
Restructure if logic
pmalek Oct 2, 2020
7c5748f
Add test covering unset pod.Spec.Hostname
pmalek Oct 2, 2020
dc18129
Revert to original set of default tags
pmalek Oct 7, 2020
9502d95
Fix options tests
pmalek Oct 7, 2020
0848767
Revert adding host.name tagging option
pmalek Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions processor/k8sprocessor/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,26 @@ func TestLoadConfig(t *testing.T) {
config, err := configtest.LoadConfigFile(
t,
path.Join(".", "testdata", "config.yaml"),
factories)
factories,
)

require.Nil(t, err)
require.NoError(t, err)
require.NotNil(t, config)

p0 := config.Processors["k8s_tagger"]
assert.Equal(t, p0,
assert.Equal(t,
&Config{
ProcessorSettings: configmodels.ProcessorSettings{
TypeVal: "k8s_tagger",
NameVal: "k8s_tagger",
},
APIConfig: k8sconfig.APIConfig{AuthType: k8sconfig.AuthTypeServiceAccount},
})
},
p0,
)

p1 := config.Processors["k8s_tagger/2"]
assert.Equal(t, p1,
assert.Equal(t,
&Config{
ProcessorSettings: configmodels.ProcessorSettings{
TypeVal: "k8s_tagger",
Expand All @@ -66,7 +69,17 @@ func TestLoadConfig(t *testing.T) {
APIConfig: k8sconfig.APIConfig{AuthType: k8sconfig.AuthTypeKubeConfig},
Passthrough: false,
Extract: ExtractConfig{
Metadata: []string{"podName", "podUID", "deployment", "cluster", "namespace", "node", "startTime"},
Metadata: []string{
"cluster",
"containerName",
"deployment",
"hostName",
"namespace",
"node",
"podName",
"podUID",
"startTime",
},
Annotations: []FieldExtractConfig{
{TagName: "a1", Key: "annotation-one"},
{TagName: "a2", Key: "annotation-two", Regex: "field=(?P<value>.+)"},
Expand All @@ -89,5 +102,7 @@ func TestLoadConfig(t *testing.T) {
{Key: "key2", Value: "value2", Op: "not-equals"},
},
},
})
},
p1,
)
}
25 changes: 25 additions & 0 deletions processor/k8sprocessor/kube/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package kube
import (
"fmt"
"regexp"
"sort"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -235,6 +236,30 @@ func (c *WatchClient) extractPodAttributes(pod *api_v1.Pod) map[string]string {
tags[r.Name] = c.extractField(v, r)
}
}

if c.Rules.HostName {
// Basing on v1.17 Kubernetes docs, when a hostname is specified, it takes precedence over
// the associated metadata name, see:
// https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-hostname-and-subdomain-fields
if pod.Spec.Hostname == "" {
tags[conventions.AttributeHostName] = pod.Name
} else {
tags[conventions.AttributeHostName] = pod.Spec.Hostname
}
Copy link
Member

Choose a reason for hiding this comment

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

hostname :=  pod.Spec.Hostname
if hostname == "" {
  hostname = pod.Name
}
tags[conventions.AttributeHostName] = hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular reasons for this? (other than maybe accessing pod.Spec.Hostname just once)

Copy link
Contributor

Choose a reason for hiding this comment

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

@pmalek-sumo, I think such approach makes the intention more clear. Currently, you check pod.Spec.Hostname and assign pod.Name which is not obvious why, unless the line below is being read

Of course, that might be a small thing, but one where clarity could be improved easily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏻 done

}

if c.Rules.ContainerNames {
if len(pod.Spec.Containers) > 0 {
var names []string
for _, container := range pod.Spec.Containers {
names = append(names, container.Name)
}

sort.Strings(names)
tags[conventions.AttributeK8sContainer] = strings.Join(names, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps ", " (with a space after the comma) to make it more clear? Or using the array (AnyValue_ArrayValue) type?

Copy link
Contributor

Choose a reason for hiding this comment

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

k8s.container.name should be about a single container. I assume we can't do that here because we correlate based on IP and the IP is shared across all the containers in the pod. If we make this an array (seems better) I think we should also consider adding a new semantic convention of k8s.pod.containers for this purpose.

Copy link
Contributor

@pmm-sumo pmm-sumo Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I looked around semantic conventions to see if there's any other attribute troubled by a similar property (usually being a single value, but sometimes being a collection). The closest I get is process.command_line, but there are really no other examples I could find.

The same problem will apply to Kubernetes Service - there might be several of them pointing to a single pod.

I am not sure if this ambiguity could be solved in any way currently (@dashpole, perhaps you might have some suggestions?).

This leads to several specification-related questions, such as:

  • should there bet two attributes (one singular, one plural) depending if there's (typically) one or (sometimes) multiple values or, only one of them?
  • if there's only one attribute name, should it be plural or singular when there might be a collection of values for a key?
  • if there's only one attribute name, what should be the type of attribute value (always an array or perhaps a string when there's only single entry)?

For reference, there's a an ongoing discussion on pluralization guidelines for metrics

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a metric about a pod (e.g. is correlated with the Pod's IP), then IMO it shouldn't have a container attribute. If it is about a container, then it should have a single container name.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dashpole so essentially when a single container cannot be accurately assigned then the information should be not included, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, at least in my opinion.

From monitoring systems i'm familiar with, I haven't seen a good way to handle lists of items in label (attribute) values.

The other thing to consider is that technically containers can be added to a running pod with the debug container feature, so this attribute wouldn't necessarily be constant over the lifetime of a pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other thing to consider is that technically containers can be added to a running pod with the debug container feature, so this attribute wouldn't necessarily be constant over the lifetime of a pod.

Even though this seems like a niche use case I agree that it might introduce incorrect data when we'd tag as above.

From monitoring systems i'm familiar with, I haven't seen a good way to handle lists of items in label (attribute) values.

As for this I was trying to approach tags rewrite to not be of map[string]string type but rather a map of string to AnyValue to have the tags already in the type that they will be serialized to.

This could make it easier as having a plural tag as key would already indicate there's an array in the value.

}
}

return tags
}

Expand Down
27 changes: 20 additions & 7 deletions processor/k8sprocessor/kube/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,15 @@ func TestExtractionRules(t *testing.T) {
},
Spec: api_v1.PodSpec{
NodeName: "node1",
Hostname: "myhostname",
Containers: []api_v1.Container{
{
Name: "container-zzzzz",
},
{
Name: "sidecar-container-aaaaa",
},
},
},
Status: api_v1.PodStatus{
PodIP: "1.1.1.1",
Expand All @@ -348,13 +357,15 @@ func TestExtractionRules(t *testing.T) {
}, {
name: "metadata",
rules: ExtractionRules{
Deployment: true,
Namespace: true,
PodName: true,
PodUID: true,
Node: true,
Cluster: true,
StartTime: true,
Deployment: true,
Namespace: true,
PodName: true,
PodUID: true,
Node: true,
Cluster: true,
StartTime: true,
HostName: true,
ContainerNames: true,
},
attributes: map[string]string{
"k8s.deployment.name": "auth-service",
Expand All @@ -364,6 +375,8 @@ func TestExtractionRules(t *testing.T) {
"k8s.pod.name": "auth-service-abc12-xyz3",
"k8s.pod.uid": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
"k8s.pod.startTime": pod.GetCreationTimestamp().String(),
"k8s.container.name": "container-zzzzz,sidecar-container-aaaaa",
"host.name": "myhostname",
},
}, {
name: "labels",
Expand Down
16 changes: 9 additions & 7 deletions processor/k8sprocessor/kube/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,15 @@ type FieldFilter struct {
// ExtractionRules is used to specify the information that needs to be extracted
// from pods and added to the spans as tags.
type ExtractionRules struct {
Deployment bool
Namespace bool
PodName bool
PodUID bool
Node bool
Cluster bool
StartTime bool
Deployment bool
Namespace bool
PodName bool
PodUID bool
Node bool
Cluster bool
StartTime bool
HostName bool
ContainerNames bool

Annotations []FieldExtractionRule
Labels []FieldExtractionRule
Expand Down
44 changes: 26 additions & 18 deletions processor/k8sprocessor/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ const (
filterOPExists = "exists"
filterOPDoesNotExist = "does-not-exist"

metdataNamespace = "namespace"
metadataPodName = "podName"
metadataPodUID = "podUID"
metadataStartTime = "startTime"
metadataDeployment = "deployment"
metadataCluster = "cluster"
metadataNode = "node"
metadataNamespace = "namespace"
metadataPodName = "podName"
metadataPodUID = "podUID"
metadataStartTime = "startTime"
metadataDeployment = "deployment"
metadataCluster = "cluster"
metadataNode = "node"
metadataHostName = "hostName"
metadataContainerNames = "containerNames"
)

// Option represents a configuration option that can be passes.
Expand Down Expand Up @@ -68,31 +70,37 @@ func WithExtractMetadata(fields ...string) Option {
return func(p *kubernetesprocessor) error {
if len(fields) == 0 {
fields = []string{
metdataNamespace,
metadataCluster,
metadataContainerNames,
metadataDeployment,
metadataHostName,
dmitryax marked this conversation as resolved.
Show resolved Hide resolved
metadataNamespace,
metadataNode,
metadataPodName,
metadataPodUID,
metadataStartTime,
metadataDeployment,
metadataCluster,
metadataNode,
}
}
for _, field := range fields {
switch field {
case metdataNamespace:
case metadataCluster:
p.rules.Cluster = true
case metadataContainerNames:
p.rules.ContainerNames = true
case metadataDeployment:
p.rules.Deployment = true
case metadataHostName:
p.rules.HostName = true
case metadataNamespace:
p.rules.Namespace = true
case metadataNode:
p.rules.Node = true
case metadataPodName:
p.rules.PodName = true
case metadataPodUID:
p.rules.PodUID = true
case metadataStartTime:
p.rules.StartTime = true
case metadataDeployment:
p.rules.Deployment = true
case metadataCluster:
p.rules.Cluster = true
case metadataNode:
p.rules.Node = true
default:
return fmt.Errorf("\"%s\" is not a supported metadata field", field)
}
Expand Down
4 changes: 4 additions & 0 deletions processor/k8sprocessor/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ func TestWithExtractMetadata(t *testing.T) {
assert.True(t, p.rules.Deployment)
assert.True(t, p.rules.Cluster)
assert.True(t, p.rules.Node)
assert.True(t, p.rules.HostName)
assert.True(t, p.rules.ContainerNames)

p = &kubernetesprocessor{}
err := WithExtractMetadata("randomfield")(p)
Expand All @@ -219,6 +221,8 @@ func TestWithExtractMetadata(t *testing.T) {
assert.False(t, p.rules.StartTime)
assert.False(t, p.rules.Deployment)
assert.False(t, p.rules.Node)
assert.False(t, p.rules.HostName)
assert.False(t, p.rules.ContainerNames)
}

func TestWithFilterLabels(t *testing.T) {
Expand Down
8 changes: 5 additions & 3 deletions processor/k8sprocessor/testdata/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ processors:
extract:
metadata:
# extract the following well-known metadata fields
- podName
- podUID
- deployment
- cluster
- containerName
- deployment
- hostName
- namespace
- node
- podName
- podUID
- startTime

annotations:
Expand Down