Skip to content

added source filtering#5881

Closed
scampi wants to merge 9 commits intoelastic:masterfrom
scampi:issue4366
Closed

added source filtering#5881
scampi wants to merge 9 commits intoelastic:masterfrom
scampi:issue4366

Conversation

@scampi
Copy link
Contributor

@scampi scampi commented Jan 12, 2016

moved to #5981

  • fetch field if:
    • explicitly selected in discover
    • show in a table
    • used in an aggregate

close #4366

I am not sure if this PR is a complete solution for the issue, but I believe it is a part of it.

This PR adds a configuration on the index pattern level that allows to define which field to exclude using the source filtering API. There is a new tab in which you can set the JSON to specify what to exclude.
The fields that are actually retrieved are indicated on the Fields tab as an additional column: if it is retrieved, the row is checked.

2016-01-12-104207_1920x1848_scrot

To test it:

  • Create a small index:
curl -XPUT 'http://localhost:9200/country/Country/1' -d '
{
  "name": "Italy",
  "description": "pasta"
}'
  • Add the country index and go to the Retrieved Fields tab. Exclude the description field from being retrieved. Add the following object to do so:
{
 "exclude": "description"
}

2016-01-12-105353_1920x1848_scrot

  • The description is now unchecked, meaning it is not retrieved.

2016-01-12-103741_1920x1848_scrot

  • Create a saved search for the countries and then add a search visualisation to a dashboard. You will see that the field description does not appear.

2016-01-12-104756_1920x1848_scrot


What is not provided in this PR but could be necessary: a way to display an excluded field for a document. Do you see this mandatory, or is the proposed solution good enough ?

@ghost
Copy link

ghost commented Jan 12, 2016

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@spalger
Copy link
Contributor

spalger commented Jan 12, 2016

@scampi am I interpreting your question properly? Should we show columns/fields for the fields that are excluded? Or should Kibana basically pretend like they don't exist?

I think we should simply exclude them when we are fetching the source for a document. If the user wants to aggregate on that field, or show it in a table, or even select it explicitly in discover then the field should be fetched.

@scampi
Copy link
Contributor Author

scampi commented Jan 12, 2016

@spalger your 2nd paragraph actually answers my question ;o)

At the moment, there is no way to retrieve a field once it is excluded. It will appear as a dash - for example in the last screenshot.

What you suggest is something that needs to be added. It will take some time. I will update this PR with it.

@spalger
Copy link
Contributor

spalger commented Jan 13, 2016

Awesome.

Would you mind getting rid of the "Retrieved Fields" tab and putting a checkbox on the field editor screen?

http://localhost:5601/app/kibana#/settings/indices/logstash-*/field/_index
image

@scampi
Copy link
Contributor Author

scampi commented Jan 13, 2016

@spalger it should be better now with the new commit

  • the retrieved field tab is removed in favor of a checkbox in the field control view
  • if a saved search uses a field that is excluded, it is retrieved anyway

2016-01-13-144849_1920x1848_scrot

@spalger
Copy link
Contributor

spalger commented Jan 13, 2016

Looking very good, will take a deeper look later today.

@spalger spalger self-assigned this Jan 13, 2016
@rashidkpc
Copy link
Contributor

jenkins, test it

@scampi
Copy link
Contributor Author

scampi commented Jan 14, 2016

@spalger I have two questions:

  1. Should we add a warning on the discover page to indicate that a field is excluded ?
    If a field is excluded, it is probably for performance reason. Therefore, we can say that selecting it might have some performance impact (because the field is large) ?
  2. On the index setting page, should we omit the checkmark logo for the fields _* like _index ?

@spalger
Copy link
Contributor

spalger commented Jan 14, 2016

@scampi

Should we add a warning on the discover page...

I don't think that's necessary. Just because the value is excluded doesn't mean that it would cause performance issues, and we probably shouldn't be discouraging users from using that field.

should we omit the checkmark logo for the fields _* like _index?

I think that it might make sense to hide disable the checkbox (with an explanation) if the field is in the config.get('metaFields')

@jccq
Copy link

jccq commented Jan 15, 2016

Just for completeness - would it make sense that in the panel showing the records fields (e.g. search widget/discover) a writing appears "Note: this record might be incomplete as some fields are currently hidden in the configuration" if needs to?

@scampi
Copy link
Contributor Author

scampi commented Jan 15, 2016

@spalger

  • do not show the checkmark icon if the field is a metafield.

2016-01-15-154026_1920x1848_scrot

  • opening the field editor on a metafield disables the checkbox (you cannot check it) and displays a warning message.
    2016-01-15-154110_1920x1848_scrot

@spalger
Copy link
Contributor

spalger commented Jan 15, 2016

@jccq I think we're going to skip on the warning for now. The way I see it is that excluding a field from an index pattern is somewhat of an "admin" action, and simply viewing the document is more of a "user" action.

I doubt much good would come from telling the user that fields were excluded; they would likely either ignore the warning or try to find a way to include them.

@scampi very nice

@scampi
Copy link
Contributor Author

scampi commented Jan 16, 2016

@spalger thanks ;o)

The test fails because the ES port was already in use. Should I open an issue so that the test framework checks if the port is free ?

MALFORMED LOG MESSAGE: "EXCEPTION IN THREAD \"MAIN\" BINDHTTPEXCEPTION[FAILED TO BIND TO [9220]]; NESTED: CHANNELEXCEPTION[FAILED TO BIND TO: /127.0.0.1:9220]; NESTED: BINDEXCEPTION[ADDRESS ALREADY IN USE];" -  - cluster - 
MALFORMED LOG MESSAGE: "LIKELY ROOT CAUSE: JAVA.NET.BINDEXCEPTION: ADDRESS ALREADY IN USE" -  - cluster - 

@spalger
Copy link
Contributor

spalger commented Jan 18, 2016

@scampi no need, we are working to improve the isolation of our jenkins builds but sometimes the collide with others.

@spalger spalger mentioned this pull request Jan 21, 2016
@spalger
Copy link
Contributor

spalger commented Jan 21, 2016

@scampi played around with this yesterday+today and submitted a few tweaks to your branch in scampi#1. If you'll take a look and consider merging then I think this is ready for final review.

@scampi
Copy link
Contributor Author

scampi commented Jan 22, 2016

i'm not sure if you got some notification @spalger , so just in case, your changes were merged.

@spalger
Copy link
Contributor

spalger commented Jan 22, 2016

👍 thanks @scampi. Passing this to @jbudz for another set of eyes.

@spalger spalger removed their assignment Jan 22, 2016
@jbudz
Copy link
Contributor

jbudz commented Jan 22, 2016

From adding a new index pattern:
image

@jbudz
Copy link
Contributor

jbudz commented Jan 22, 2016

This doesn't seem to be filtering any fields for me,
image
image

@spalger
Copy link
Contributor

spalger commented Jan 22, 2016

In order to iterate on this a bit I'm moving this to #5981

@scampi scampi deleted the issue4366 branch January 23, 2016 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loading huge documents

5 participants

Comments