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

Use safe put for k8s metadata #6490

Merged
merged 1 commit into from
Mar 13, 2018
Merged

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Mar 1, 2018

This makes use of #6434 as an alternative to avoid mapping issues while keeping dots in metadata.

Before this change, label sets breaking nesting would result in mapping errors, for example this one:

co.elastic.task: "x"
co.elastic.task.id: 1
co.elastic.task.name: "foobar"

Now we do preemptive renaming to avoid errors (.value is appended to co.elastic.task):

co.elastic.task.value: "x"
co.elastic.task.id: 1
co.elastic.task.name: "foobar"

@exekias
Copy link
Contributor Author

exekias commented Mar 7, 2018

jenkins, retest it please

@@ -114,7 +115,7 @@ func generateMapStrFromEvent(eve *kubernetes.Event) common.MapStr {
if len(eve.Metadata.Annotations) != 0 {
annotations := make(common.MapStr, len(eve.Metadata.Annotations))
for k, v := range eve.Metadata.Annotations {
annotations[common.DeDot(k)] = v
safemapstr.Put(annotations, k, v)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should document this under breaking changes? As in case someone filtered for the annotations this queries now stop working. Will still merge the PR but we can discuss this as a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

Note: I agree this is a bug fix in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I wonder how we should communicate this, so far without this change the only way to get some field that is renamed by this was using drop_field processor to ensure there are no mapping issues.

Copy link
Member

Choose a reason for hiding this comment

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

As we link to the PR in the changelog could you add more details to the PR message? So people ending up on this PR get some background info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, done!

@ruflin ruflin merged commit 75e717c into elastic:master Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants