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 1 commit
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 len(pod.Spec.Containers) > 0 {
if c.Rules.ContainerName {
pmalek-sumo marked this conversation as resolved.
Show resolved Hide resolved
var names []string
for _, container := range pod.Spec.Containers {
names = append(names, container.Name)
}

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

Choose a reason for hiding this comment

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

According to semantic conventions
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/k8s.md#container, if we are using k8s spec as the source, it's not a real container name. container.name supposed to be provided by container engine. We should use k8s.container.name here instead

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 but please comment whether it makes sense to have configuration option in plural and the tag in singular.

}
}

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,
ContainerName: 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(),
"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
ContainerName bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Would ContainerNames make more sense as we add all names we see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this but taking into account @dmitryax's comment I'm not sure it will fit since the tag k8s.container.name is in singular.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep it singular here for consistency, even if it may have multiple values

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. Singular it is.


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"
metadataContainerName = "containerName"
)

// 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,
metadataContainerName,
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 metadataContainerName:
p.rules.ContainerName = 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.ContainerName)

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.ContainerName)
}

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