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

Set language for scripted field #7700

Merged
merged 4 commits into from
Aug 29, 2016
Merged

Set language for scripted field #7700

merged 4 commits into from
Aug 29, 2016

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jul 9, 2016

Fixes #6529
Based on the work started in #7206

This PR allows Kibana scripted fields to be written in any language that has been enabled for inline scripting in ES. By default, that's expression and painless in 5.0 (and technically mustache, but I don't expose it as an option since it makes no sense in script_fields). Groovy and other languages can be enabled in the Elasticsearch config. However, I've spent the most time trying to optimize the experience for painless and expression since those are the two sandboxed languages we should be encouraging people to use.

If you'd like to enable groovy in dev mode, you can add the following code to your grunt esvm (esvm.js) dev config block:

script: {
  engine: {
    groovy: {
      inline: true
    }
  }
}

Here's a screenshot of how things should look when it's running:

screen shot 2016-08-19 at 4 01 49 pm

Todo:

  • Test scripted fields with various field types
  • Restrict type options to those that actually work with scripted fields
  • Formatter list not getting updated when type is changed
  • Unit tests
  • API tests
  • Update in page docs
  • Date type scripted fields getting added to fielddata_fields in Discover
  • Restrict type based on language selected? (e.g. only numbers when expression lang is used)
  • Update (Default: Number) field label when type updates
  • Make painless the default lang

Scripted Field Types (from all possible Kibana field types):
string (yes)
date (yes, but needs fix for issue above)
boolean (yes)
number (yes)
geo_point (no, geohash_grid doesn't support scripts)
geo_shape (no)
ip (doesn't seem to work at the moment elastic/elasticsearch#20067)
attachment (doesn't seem useful)
murmur3 (only comes with a plugin, probably not worth supporting OOB)

@thomasneirynck
Copy link
Contributor

This is somewhat of a nice to have, and can be split up from this PR.

But it would be helpful if the scripted field was validated when editing/creating it. Right now, you have to use it in a viz before you can tell if something's wrong.

e.g., following compilation error could be found at design time:

  • either by testing it on the first document (less hard)
  • inspecting the index mapping (harder)
    image

Is there API in ES to test/compile a script before running it?

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Aug 22, 2016

I like that the forms update automatically depending on the selection. E.g. the expression language only gives you the number as a possible return type, while with painless you have multiple options. It's a good safety mechanism, but it is not very intuitive when changing something on the bottom of the form, changes input on the top.

Since these dependencies cascade down, I would move up the script-language selection to the top of the form, since it governs so much of what comes below (Language > Type > Format).

(You'll want to check this off with a UX expert too though. Apart from moving language to the top, I would even move up the script-field to the 2nd row. It's the most important field after all and determines if all the rest under it is valid).

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Aug 22, 2016

I'm not a fan of the popularity field in "scripted fields".

It's purpose is not explained with a warning-blurb, or a tooltip. It is not explained in any of the three links on the page (script fields, scripts in aggregations, lucene expressions).

In the Kibana documentation, we can learn it is used to sort the fields in the field-list. But then I'm not 100% it actually works (see #5029).

Since we have such a nice free text filter search bar, and I don't expect scripted fields to exceed more than 25 (or perhaps they do (?)), I would just remove the option from the form altogether.

@@ -6,7 +6,7 @@ export default function () {
let scriptFields = {};
let docvalueFields = [];

docvalueFields = _.pluck(self.fields.byType.date, 'name');
docvalueFields = _.pluck(_.reject(self.fields.byType.date, 'scripted'), 'name');
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, plucks deprecated in lodash from v4 onwards, so if we ever upgrade, we'll have to remove usage. (use 'map' instead).

@Bargs
Copy link
Contributor Author

Bargs commented Aug 23, 2016

@thomasneirynck I think I've addressed all your comments. A few replies:

  • Script validation - agree, nice to have, but makes sense to do it as a separate enhancement. I don't think there's any script validation API in ES atm
  • Language input location - totally agree, I've moved it to the top, just under the name input. I like having script at the bottom though, since it can expand in size, and it's where users are used to seeing it.
  • popularity - it affects sorting in the Discover sidebar, and as far as I can tell it works. Agreed that it could be explained better, but I think that belongs in another PR

Please take another look!

@thomasneirynck
Copy link
Contributor

@Bargs

  • script validation: I'll log an enhancement for that
  • language input location: ok
  • popularity: OK. I'm like Donny here, no frame of reference, like a child who wanders in the middle of a movie. The popularity field does work as intended indeed. What is "broken" (Improve sorting on popularity #5029) is that sort order isn't respected in the field editor-list, and after editing, fields are pushed to the back of the list making them "disappear".

OK, I'll look it over once more, thanks!

@Bargs
Copy link
Contributor Author

Bargs commented Aug 23, 2016

Just pushed a couple functional tests, and with that I've finished all the "code", pending any bugs or updates based on review of course. Just gotta work on the in page docs now.

@thomasneirynck
Copy link
Contributor

jenkins, test this.

@Bargs
Copy link
Contributor Author

Bargs commented Aug 23, 2016

I think there are unrelated test failures caused by the ESVM refresh last night :(

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Aug 23, 2016

LGTM

I like this improvement! scripted fields are a cool feature of ES, and it's nice to see it propagate all the way to the UI!

@Bargs Bargs changed the title [WIP] Fix #6529 Set language for scripted field Fix #6529 Set language for scripted field Aug 23, 2016
@tsullivan tsullivan self-assigned this Aug 23, 2016
@tbragin
Copy link
Contributor

tbragin commented Aug 23, 2016

From a functional perspective, I tried a number of Lucene expressions and Painless scripted fields and so far these worked well. Working with the getting-started dataset for testing purposes. Capturing them here in case other testers want ideas for things to try, or for potential examples to include in the docs / blogs.

Lucene expression - Number math on a field
doc['bytes'].value / 1024

Lucene expression - Parse date into DOW, etc.. (into number)
doc['@timestamp'].date.dayOfWeek

Painless - Combine two string values
doc['machine.os.keyword'].value + ' ' + doc['extension.keyword'].value

Note: Starting with 5.0-rc1, you have to set the following setting in elasticsearch.yml to turn on regex matching for Painless:
script.painless.regex.enabled: true

Painless - Match a string using regex, and take action on a match
if (doc['referer.keyword'].value =~ /error/) { return "error"} else {return "no error"}

Painless - Match a string and return that match

def m = /^.*\.([a-z]+)$/.matcher(doc['host.keyword'].value);
if ( m.matches() ) {
   return m.group(1)
} else {
   return "no match"
}

Painless - Parse date into DOW, etc.. (into string)
LocalDateTime.ofInstant(Instant.ofEpochMilli(doc['@timestamp'].value), ZoneId.of('Z')).getDayOfWeek().getDisplayName(TextStyle.FULL, Locale.getDefault());

screen shot 2016-08-23 at 9 54 26 am

@tbragin
Copy link
Contributor

tbragin commented Aug 23, 2016

From a usability perspective, it would be nice if the script field box automatically expanded to the size of the script, since painless scripts tend to be longer. If that's a reasonable idea, but should be handled as part of a separate PR, happy to file a ticket.

Default view:
screen shot 2016-08-23 at 10 14 16 am

Manually expanded:
screen shot 2016-08-23 at 10 14 25 am

I also agree with @thomasneirynck and @ppf2 on the fact that having additional validation and preview of script effects on a sample document would help from a workflow development perspective, as a future enhancement:
#8057
#2781

</h4>

<p>
By default, Kibana scripted fields use <a target="_window" ng-href="{{editor.docLinks.painless}}">Painless <i class="fa-link fa"></i></a>, a simple and secure scripting language designed specifically for use with Elasticsearch. To access values in the document use the following format:
Copy link
Member

@tsullivan tsullivan Aug 23, 2016

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was rushing to get that last commit in, meant to add a note about that. These should be valid links once an actual 5.0 branch exists in ES. ^ That's still correct, isn't it @clintongormley?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but if you merge this change before we have a 5.0 branch it will break the docs build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll disable the kibana link checking until the ES has a 5.0 branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @clintongormley!

@nik9000
Copy link
Member

nik9000 commented Aug 23, 2016

def m = /^.*\.([a-z]+)$/.matcher(doc['host.keyword'].value); if ( m.matches() ) { return m.group(1) } else { return "no match" }

I think we should push people to put new lines in their scripts with examples like:

def m = /^.*\.([a-z]+)$/.matcher(doc['host.keyword'].value);
if (m.matches()) {
  return m.group(1);
} else {
  return "no match";
}

I like new lines....

@tbragin
Copy link
Contributor

tbragin commented Aug 23, 2016

Another thing that might be useful is ability to disable scripted fields -- say if one is slow or isn't working. Right now the only way to stop them from running is to delete them.

@epixa epixa changed the title Fix #6529 Set language for scripted field Set language for scripted field Aug 24, 2016
@Bargs
Copy link
Contributor Author

Bargs commented Aug 24, 2016

Ah great catch @tbragin, I missed those first two because I didn't modify the warning message at all. I've updated those first two to use the same dynamic doc linking strategy as the others. They'll 404 at the moment because there's no ES 5.0 branch (see comments from Clint in the inline comments above). You can validate that the rest of the URL is correct by replacing 5.0 with master in the URL.

import _ from 'lodash';
import handleESError from '../../../lib/handle_es_error';

export function registerLanguages(server) {
Copy link
Member

Choose a reason for hiding this comment

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

not an issue, but I'm wondering why it's not an export default here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure, just following the pattern of the other route registration modules in this plugin.

return request.get('/kibana/scripts/languages')
.expect(200)
.then(function (response) {
expect(response.body).to.be.an('array');
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked much at this kind of test code before. Does it actually query the server connected to Elasticsearch and get an actual response, or are there mocks somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No mocks, these are full integration tests.

@tsullivan
Copy link
Member

code LGTM, just had a few questions

@Bargs
Copy link
Contributor Author

Bargs commented Aug 24, 2016

@tsullivan @thomasneirynck @tbragin I made one more update to show an error message and disable the scripted field creation form when inline scripting is disabled in ES:

screen shot 2016-08-24 at 7 33 11 pm

Could you all take one more quick look?

There appears to be a bug in ES's reporting of enabled languages when you simply disable all scripting, so to test for now you'll need to disabled painless and expression individually

You can do that like this in your grunt esvm.js file:

    dev: {
      options: {
        directory: resolve(directory, 'dev'),
        config: {
          path: {
            data: dataDir
          },
          cluster: {
            name: 'esvm-dev'
          },
          script: {
            engine: {
              painless: {
                inline: false
              },
              expression: {
                inline: false
              }
            }
          }
        }
      }
    },

delete self.scriptingLangs value assign and correct a typo
@Bargs
Copy link
Contributor Author

Bargs commented Aug 25, 2016

jenkins, test this

@Bargs
Copy link
Contributor Author

Bargs commented Aug 26, 2016

Tests finally passed, this is g2g once I get a final LGTM on that last change I made

@Bargs Bargs added the review label Aug 26, 2016
@thomasneirynck
Copy link
Contributor

LGTM

For future consideration: I think that warning message would also be appropriate to put on the "Scripted Fields" tab of Index-pattern overview, since it is a global setting.

@thomasneirynck thomasneirynck removed their assignment Aug 26, 2016
@Bargs
Copy link
Contributor Author

Bargs commented Aug 26, 2016

Thanks @thomasneirynck! I'm gonna squash and merge Monday morning.

@epixa
Copy link
Contributor

epixa commented Aug 26, 2016

Is that unchecked todo item still pending?

@Bargs
Copy link
Contributor Author

Bargs commented Aug 26, 2016

I couldn't reproduce it, I think it may have been an unrelated issue that got fixed elsewhere. I mostly left it there to remind myself to keep an eye out, but I guess I can remove it at this point.

Bargs added 2 commits August 29, 2016 11:26
To provide additional language options for scripted fields, Kibana needs
to know what languages are available for inline scripting in
Elasticsearch. This new Kibana API grabs up to date information from
Elasticsearch and provides it in an easy to digest format for the UI.

The response body from the API is a simple JSON array with a list of
languages that are enabled for inline scripting. The API also filters
out languages that don't make sense in scripted fields, like 'mustache'.

An example request might look like this:

`GET /api/kibana/scripts/languages`

200 OK
`['expression', 'painless']`
This commit allows Kibana scripted fields to be written in any language
that has been enabled for inline scripting in ES. By default, that's
expression and painless in 5.0. Groovy and other languages can be
enabled in the Elasticsearch config. However, I've spent the most time
trying to optimize the experience for painless and expression since
those are the two sandboxed languages we should be encouraging people
to use.

Field type options are limited per language based on what makes sense
for that language and for scripte fields in general. For instance,
expression scripts can only be number fields because expressions only
return numbers. No scripted fields can be geo_points, because the geo
aggregations Kibana uses don't support scripts.

This UI enhancement uses the new script languages API added in the
previous commit to get the list of inline scripting languages
dynamically.
@Bargs Bargs merged commit 5bbe02e into elastic:master Aug 29, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Set language for scripted field

Former-commit-id: 5bbe02e
jbudz pushed a commit that referenced this pull request May 10, 2024
`v94.2.1-backport.0` ⏩ `v94.3.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v94.3.0`](https://github.com/elastic/eui/releases/v94.3.0)

- Updated `launch` glyph for `EuiIcon`
([#7670](elastic/eui#7670))
- Updated `EuiComboBox`'s `options` to support including tooltip details
for selectable options. Use `toolTipContent` to render tooltip
information, and `toolTipProps` to optionally customize the tooltip
rendering behavior ([#7700](elastic/eui#7700))
- Updated the following existing glyphs in `EuiIcon`:
([#7727](elastic/eui#7727))
  - `error` (now an outlined version instead of filled) 
  - `tokenMetricCounter`
  - `tokenMetricGauge` 
- Added the following new glyphs to `EuiIcon`:
([#7727](elastic/eui#7727))
  - `tokenDimension`
  - `clickLeft`
  - `clickRight`
  - `clockCounter`
  - `errorFilled` (the previous `error` glyph design)
  - `warningFilled`

**Bug fixes**

- Fixed a visual layout bug for `EuiComboBox` with `isLoading` in mobile
views ([#7700](elastic/eui#7700))
- Fixed missing styles on header cells of `EuiDataGrid` that prevented
content text alignment styles to apply
([#7720](elastic/eui#7720))
- Fixed `EuiFlexGroup` and `EuiFlexItem` `ref` prop typing to support
refs of the same type as the passed `component` type and allow
`displayName` to be defined for easy component naming when using
component wrappers like `styled()`
([#7724](elastic/eui#7724))

---

Most of the code changes you'll see in this PR are caused by the recent
EuiFlex* changes making it generic. This, unfortunately, is something
that `styled()` doesn't always like. I replaced the failing usages of
`styled(EuiFlexGroup)` and `styled(EuiFlexItem)` to use `component` and
other native EuiFlex* props, resulting in the same output but being
better typed.

We plan to add more props to EuiFlex* components giving developers
control over properties like `flex-grow` and `flex-shring`, and reducing
the need for writing any custom CSS when using these components. This
should reduce the number of `styled()` wrappers needed even further

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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.

Provide ability to set language for a scripted field
8 participants