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

Dedot kubernetes fields #6203

Merged
merged 4 commits into from
Jan 31, 2018
Merged

Dedot kubernetes fields #6203

merged 4 commits into from
Jan 31, 2018

Conversation

walktall
Copy link
Contributor

Related to #5942

@ruflin @exekias PTAL~

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

result[DeDot(key)] = DeDotJSON(value)
}
return result
case MapStr:
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this option here. Could you add a test case for this "switch" option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin Tests added, and also made some changes to original test code...

@@ -54,11 +54,11 @@ func (g *metaGenerator) PodMetadata(pod *Pod) common.MapStr {
}

if len(labelMap) != 0 {
meta["labels"] = labelMap
meta["labels"] = common.DeDotJSON(labelMap)
Copy link
Member

Choose a reason for hiding this comment

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

@exekias This is a breaking change. I wonder if you see an issue in this?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need this as a bugfix anyway, isn't it? Didn't follow previous conversations but I guess this is needed to avoid mapping issues?

In any case it's a breaking change and we should make it clear in the release notes

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know it didn't break yet but it would pretty soon :-) So yes having it as breaking change caused by a bug is a good way to state it.

@ruflin
Copy link
Member

ruflin commented Jan 27, 2018

jenkins, test it

@ruflin ruflin requested a review from exekias January 27, 2018 21:09
@ruflin
Copy link
Member

ruflin commented Jan 27, 2018

jenkins, test it

input []byte
output []byte
valuer func() interface{}
}{
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for converting this to a table test. It makes it much easier to compare input with output. It makes it a bit harder to write json then before but I think the benefits outweight here.

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

thank you for taking this!

@exekias
Copy link
Contributor

exekias commented Jan 29, 2018

jenkins, test it please

@ruflin ruflin merged commit d6f899d into elastic:master Jan 31, 2018
@ruflin
Copy link
Member

ruflin commented Jan 31, 2018

@walktall Thanks for your work on this.

@exekias We should make sure this is communicate properly for 6.x in case people had queries on the label keys which will not work anymore.

@walktall walktall deleted the dedot-k8s-fields branch February 4, 2018 07:57
@vjsamuel
Copy link
Contributor

There should be a better way to handle this as it invokes disparity between how objects are represented in Kubernetes and how autodiscover is configured in Beats. For example a label prometheus.io.scrape would now have to be configured as prometheus_io_scrape. if such conversions need to happen they should ideally happen either in a configurable manner where dedoting can be turned off or later in the pipeline in the form of a processor. Not all backends may need this dedoting to happen and there maybe usecases where everything appears the way it actually is on Kubernetes.

@exekias
Copy link
Contributor

exekias commented Feb 18, 2018

I'm ok with making this optional, will open a PR for it 👍 I want to fix the way we handle metadata with dots too

@walktall
Copy link
Contributor Author

@vjsamuel
Copy link
Contributor

@walktall, autodiscover doesnt use this as it presets all the pod meta. so it would fail for that flow.

@ruflin
Copy link
Member

ruflin commented Feb 18, 2018

The problem with the dotted keys is that they also caused some issues in the processors. But this should be solved by #5916

I agree we should make it optional as I would also prefer to have the same representation in ES as is delivered by k8s. It creates the potential issue of collisions as also discussed in micrometer-metrics/micrometer#154 (comment) My thinking here is that Metricbeat should have a way prevent such collisions at least for one single event. Meaning if there is {"a": 15, "a.b": 17} that it makes something like {"a.value": 15, "a.b": 17} out of it. Would be great to have a very efficient way of doing that.

+1 on having a processor for it. The important part with a processor is that we can specify which part of an event should be dedotted as in most cases it's only the labels.

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.

5 participants