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

Conversation

@exekias
Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2018

jenkins, retest it please

@exekias exekias force-pushed the exekias:safemapstr-kubernetes branch from 5b0c941 to f077e20 Mar 12, 2018
@exekias exekias force-pushed the exekias:safemapstr-kubernetes branch from f077e20 to 2769964 Mar 12, 2018
@@ -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)

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 13, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 13, 2018

Collaborator

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

This comment has been minimized.

Copy link
@exekias

exekias Mar 16, 2018

Author Member

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.

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 16, 2018

Collaborator

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?

This comment has been minimized.

Copy link
@exekias

exekias Mar 19, 2018

Author Member

good point, done!

@ruflin ruflin merged commit 75e717c into elastic:master Mar 13, 2018
6 checks passed
6 checks passed
CLA Commit author is a member of Elasticsearch
Details
beats-ci Build finished.
Details
codecov/patch 33.33% of diff hit (within 100% threshold of 63.13%)
Details
codecov/project 63.17% (+0.03%) compared to 6176bb5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@exekias exekias referenced this pull request Mar 14, 2018
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.