Skip to content
This repository has been archived by the owner on Oct 30, 2021. It is now read-only.

Use kibiUtils.getValuesAtPath in favor of _.get #106

Merged
merged 11 commits into from
Mar 12, 2017

Conversation

RubieV
Copy link

@RubieV RubieV commented Jan 16, 2017

This adds the feature to get arrays from the es result document, by reusing kibiUtils.getValuesAtPath with .split('.'). This feature might be implemented on a higher level of abstraction, to get rid of params.labelFieldSequence statements altogether.

This adds the feature to get arrays from the es result document, by reusing kibiUtils.getValuesAtPath with .split('.'). This feature might be implemented on a higher level of abstraction, to get rid of params.labelFieldSequence statements altogether.
@sindicetech-jenkins
Copy link

Can one of the admins verify this patch?

@szydan
Copy link
Contributor

szydan commented Jan 17, 2017

jenkins test it

@RubieV
Copy link
Author

RubieV commented Jan 17, 2017

Failing: Kibi Timeline Kibi Timeline TimelineHelper pluckLabel should return the label of an event kibana-style
Will update tests to make it pass.

@scampi
Copy link
Contributor

scampi commented Jan 17, 2017

Thanks for the patch. The parameter labelFieldSequence will not be removed because it is used for supporting dots in field names, although it is a controversial feature.

@RubieV
Copy link
Author

RubieV commented Jan 17, 2017

@scampi I prefer to keep labelFieldSequence, and remove the Kibana's labelField to also make use of labelFieldSequence. It seems that the existence of the fields variable marks the difference between Kibi and Kibana. Is there a helper function for this?

When confirmed, we can move to only use labelFieldSequence uniformly.

Initialisation:

labelFieldSequence: fields[i].byName[group.labelField].path,
startFieldSequence: fields[i].byName[group.startField].path,
endFieldSequence: group.endField && fields[i].byName[group.endField].path || [],
//kibana params
labelField: group.labelField,
startField: group.startField,
endField: group.endField,

Usage:

if (params.labelFieldSequence) { // in kibi, we have the path property of a field
field = kibiUtils.getValuesAtPath(hit._source, params.labelFieldSequence);
} else {
field = _.get(hit._source, params.labelField);
}

@RubieV
Copy link
Author

RubieV commented Jan 17, 2017

781b573 fixes case where there is not split on dot, forces array creation.

@scampi
Copy link
Contributor

scampi commented Jan 17, 2017

This feature might be implemented on a higher level of abstraction, to get rid of params.labelFieldSequence statements altogether.

You meant Kibana ? If so, sorry I misread this. In kibana the support for objects in arrays has been improved and so it shouldn't be needed to use the kibiUtils version. However, in Kibi we support dots in fieldnames which kibana will not. If we choose to still support this, then those statements are still needed.

@scampi
Copy link
Contributor

scampi commented Jan 17, 2017

jenkins test it

@RubieV
Copy link
Author

RubieV commented Jan 17, 2017

Sorry for the late commits, I've updated the build artifact and added test coverage for Kibana. I had issues running the tests, so please let Jenkins do a check.

@scampi
Copy link
Contributor

scampi commented Jan 17, 2017

jenkins test it

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

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

Please remove the zip file from the PR

field = _.get(hit._source, params.labelField);
} else { // create kibi path property from ES path by splitting on .
const kibanaSplit = params.labelField.split('.');
const kibanaSequence = Array.isArray(kibanaSplit) ? kibanaSplit : Array.of(kibanaSplit);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is needed. The split always returns an array.

Left kibanaSequence as const to clarify code
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2017

CLA assistant check
All committers have signed the CLA.

@szydan
Copy link
Contributor

szydan commented Jan 27, 2017

jenkins test it

@scampi
Copy link
Contributor

scampi commented Feb 1, 2017

@RubieV please accept the PR RubieV#1

@RubieV
Copy link
Author

RubieV commented Feb 3, 2017

Accepted

@szydan
Copy link
Contributor

szydan commented Feb 3, 2017

jenkins test it

@RubieV
Copy link
Author

RubieV commented Feb 14, 2017

Sorry to bother, any updates on the status?

@scampi scampi changed the base branch from master to getValuesAtPath March 12, 2017 10:31
@scampi scampi merged commit 8da5745 into sirensolutions:getValuesAtPath Mar 12, 2017
@scampi
Copy link
Contributor

scampi commented Mar 12, 2017

@RubieV finishing this in #123 thanks

@scampi scampi mentioned this pull request Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants