allows more flexible timelion queries #10961

ppisljar · 2017-03-30T12:52:33Z

allows more flexible usage of whitespace in timelion queries.

this query now executes well:

.es     (    404  )   .yaxis  (   min   :   10   ) ;                 .es(*)      .yaxis    (    2 ,   min   =   10   )   ,  .es( 500)

adds the option to use ; instead of , but makes one of the separators obligatory (just white space is not allowed any more)

epixa · 2017-03-30T12:57:08Z

Not support whitespace anymore is going to be a problem for existing saved expressions, no? Unless there was some sort of in-place shim to support whitespace, this is a backwards compatibility break, and something we should consider an upgrade path for for 6.0.

Why is this a blocker, by the way?

cjcenizal · 2017-03-30T18:00:10Z

@epixa This is a blocker for multi-line expressions (#8881), because multi-line expressions require whitespace. However, whitespace is currently interpreted as a delimiter between expressions. So, we need to remove this interpretation of whitespace, to allow for its use strictly as an organizational tool.

This is definitely a BWC break (as noted in #10907), but the use of whitespace as a delimiter isn't documented anywhere, so it's possible that not many people use it for that purpose. My suggestion is to display a warning to the user about the change and suggest using semicolons or commas to delimit expressions as a fix. We can delay to 6.0 if you want.

epixa · 2017-03-30T20:05:49Z

Assuming I understand you correctly, I think that's a good plan. Show deprecation notices for missing commas in 5.4, and remove their support entirely in 6.0.

If that complicates this feature, then just ship this feature in 6.0.

ppisljar · 2017-03-31T05:28:51Z

i could catch the error thrown by parser and in case of Error Timelion: Error: in cell #1: metric.split is not a function show the deprecation warning.
but i have no way of telling if this same error is also thrown in some other scenarios.

what do you think ? good enough ?

epixa · 2017-03-31T14:00:30Z

@ppisljar I don't understand why we'd need to catch the error. We would just need to identify when a space is used between two expressions and show a deprecation notice. This PR would then only go into 6.0.

cjcenizal · 2017-03-31T15:56:11Z

Correct me if I'm wrong @ppisljar, but I think the reason we need to catch the error is because we don't know when a space is used between two expressions, or between two functions in a single expression, or in a query to ES. So we need to rely on PEG's parsing of the expression to know if an expression is invalid (possibly due to our BWC breaking change).

If this understanding is correct, then I think we should show our deprecation notice when the error is thrown. It's OK if this error is thrown and we show the notice, even if the error is thrown due to some reason other than this change. We can play with the wording of the notice so that it still makes sense. But this error needs to be reliably thrown in all scenarios where this change creates an invalid expression or changes the meaning of an expression.

cjcenizal · 2017-03-31T16:02:31Z

@ppisljar Can you add tests for this to verify which expressions are accepted and which throw errors?

whitespace used between two expressions
whitespace between two functions in a single expression
whitespace in a query to ES
~~semicolon-delimited expressions~~ (ignore this, per @rashidkpc's suggestion)
comma-delimited expressions
any other variations you can think of

ppisljar · 2017-03-31T16:12:32Z

@cjcenizal thats correct, regarding the parsing. I'll start adding tests.

rashidkpc · 2017-03-31T16:31:38Z

Get rid of the semi-colon support. It add complication and ambiguity. Part of the reason for getting rid of whitespace delineation was to remove ambiguity, no reason to re-introduce it.

rashidkpc · 2017-03-31T16:35:07Z

The pull description says this executes correctly:

.es     (    404  )   .yaxis  (   min   :   10   ) ;

If that is true, then this is a bug, colons are not an acceptable argument assignment character. Some functions implement them for the purpose of string splitting, but they are not part of the grammar and shouldn't be.

rashidkpc · 2017-03-31T16:39:29Z

Given that the purpose of #10907 is to make possible multi-line expressions, this would be a good time to introduce newlines and carriage returns as acceptable whitespace characters into the grammar. See here: elastic/timelion@5b21a86#diff-cad677e1524a089d5994bbffedf06119

ppisljar · 2017-04-03T09:50:31Z

@rashidkpc the min:10 doesn't actually work, but it doesn't throw an error either. however its not related to this PR.

i added the semicolon just to see what are the opinions on it ... i would prefer a semicolon there if we introduce the multiline editor ...

.es(*)
  .yaxis(1);

.es(404)
  .yaxis(2);

vs

.es(*)
  .yaxis(1),

.es(404)
  .yaxis(2),

rashidkpc · 2017-04-03T15:29:19Z

The semi-colon has no additional function over the comma. We should be making the expressions less ambiguous, not more.

Also, in your previous example, if that trailing comma is required, I'd consider that a bug.

cjcenizal · 2017-04-03T16:22:43Z

@ppisljar If you feel strongly about the semi-colons, can we introduce and debate their merits in a separate PR?

rashidkpc · 2017-04-03T17:10:59Z

@ppisljar This should throw an error if we're requiring comma delimiting as spaces aren't valid in unquoted strings. Currently its parsed as 3 arguments.

.yaxis  (   min   :   10   )

For example:

.es(
  q = html,
  index = logstash*
)

ppisljar · 2017-04-06T09:44:29Z

i removed the semicolon support and added new line support

@rashidkpc

yeah comma at the end is necessary in this version ... i have no idea how to make comma in the middle a must but comma at the end optional ...

if you feel like doing this feel free to submit another PR and close this one.

epixa · 2017-04-10T19:33:04Z

I removed the blocker label from this PR so it's not confused as a blocker for the 5.4.0 release. I added a blocked label to the issue that is blocked on this PR.

rashidkpc · 2017-04-10T20:34:26Z

src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js

+        timezone: 'Europe/Berlin'
+      }
+    };
+    return await Promise.all(chainRunner.processRequest(request));


This doesn't belong in this file, as it is testing the interpreter. You want to test that the grammar is generating a parser that is producing the expected AST

This will make the tests run faster, as well as make the tests more useful to the reader by conveying that we are only testing the parser.

This will also keep you from having to mock the time property.

rashidkpc · 2017-04-10T20:36:00Z

src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js

+    const data = ['.es() .es(404)'];
+    expect(function () {
+      parse(data);
+    }).to.throw();


You shouldn't be calling up the interpreter to test this, rather get the properties on the AST. I explain the differences in the generated AST here:

#10961 (comment)

I should've been more specific in that comment. While interpreter tests would be nice the goal of this pull is only related to parsing and the tests should be testing the parser aspect.

tbragin · 2017-04-11T19:04:18Z

Removed the 5.4.0 label, since this pull involves a breaking change in syntax (commas to be required, whereas previously they were optional). Seems like the kind of change we should be introducing in a major release, rather than minor. With 6.0 alphas around the corner, I'd recommend we target it to one of those.

This make commas requires between arguments and series, but optional at end of either. It would be simple to drop the ','? at the end of each of `arg_list` and `series` if you wanted to require the commas *not* be present.

thomasneirynck · 2017-04-24T15:27:37Z

jenkins, test this

thomasneirynck · 2017-04-24T16:06:27Z

src/core_plugins/timelion/public/chain.peg

@@ -21,7 +21,7 @@
 }

 start
- = tree:series+ {


any reason why this is removed? Don't entirely see the relation with the other changes.

series now return an array see here

thomasneirynck · 2017-04-24T16:07:56Z

src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js

+
+const expect = require('chai').expect;
+
+describe('timelion parse_sheet function', function () {


we should add tests for the change in white-space behavior too (not just for the new comma-separated behavior).

we do test for the white space behavior in the test below (it should not be allowed)

or do you mean testing that \n and \r are also accepted as white space ?

yes, test that \n and \r are now accepted.

but not sure if we already have tests in place. if not, this could be a separate effort.

thomasneirynck · 2017-04-24T16:08:41Z

src/core_plugins/timelion/public/chain.peg


 group
- = '(' space? grouped:series+ space? ')' functions:function* {


ppisljar · 2017-04-26T07:12:12Z

@rashidkpc do you want to take another look ?

rashidkpc · 2017-04-27T19:43:06Z

LGTM

rashidkpc · 2017-04-27T19:53:43Z

A quick note: white space before the '.' will cause autocomplete to not trigger. Probably not a deal breaker as implementing the multi-line editor will require a reboot of the auto completion system anyway.

cjcenizal

Awesome! Just had a small suggestion.

cjcenizal · 2017-04-27T22:12:21Z

src/core_plugins/timelion/server/handlers/__tests__/parse_sheet.js

+    expect(ast[0][0].type).to.equal('chain');
+    expect(ast[0][1].type).to.equal('chain');
+  });
+});


I was a little confused by the use of "allow" and "does not allow" in the test descriptions, because I assumed we'd be throwing errors or somehow preventing disallowed input. After I dug into the tests, I realized this was an incorrect assumption. I think it will be a little clearer if we describe the behavior as either "splitting expressions" or "not splitting expressions". Thoughts?

Because I'm not familiar with the structure of the generated AST, I also found it useful to break out the part of the object that represents the list of expressions into a variable, to increase readability.

const parseSheet = require('../lib/parse_sheet'); const expect = require('chai').expect; describe('timelion parse_sheet function', function () { it(`doesn't split expressions on whitespace`, async function () { const data = ['.es() .es(404)']; const ast = parseSheet(data); const expressions = ast[0]; expect(expressions.length).to.equal(1); expect(expressions[0].type).to.equal('chain'); });] it('splits expressions on commas', function () { const data = ['.es(), .es(404)']; const ast = parseSheet(data); const expressions = ast[0]; expect(expressions.length).to.equal(2); expect(expressions[0].type).to.equal('chain'); expect(expressions[1].type).to.equal('chain'); }); it('splits expressions on newlines', function () { const data = [`.es()\n\r ,\n\r .es(404)`]; const ast = parseSheet(data); const expressions = ast[0]; expect(expressions.length).to.equal(2); expect(expressions[0].type).to.equal('chain'); expect(expressions[1].type).to.equal('chain'); }); });

epixa · 2017-05-01T23:22:32Z

@ppisljar I'm not sure where this landed - is it still a breaking change? If so, can you please update the breaking change docs for 6.0 with details?

ppisljar · 2017-05-03T10:06:43Z

@epixa thanks for the reminder! it landed in 6.0

ppisljar added Feature:Timelion Timelion app and visualization Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.4.0 v6.0.0 labels Mar 30, 2017

ppisljar assigned cjcenizal and thomasneirynck Mar 30, 2017

ppisljar added the blocker label Mar 30, 2017

cjcenizal assigned rashidkpc Mar 31, 2017

thomasneirynck self-requested a review April 5, 2017 01:02

thomasneirynck removed their assignment Apr 5, 2017

rashidkpc self-requested a review April 5, 2017 16:47

rashidkpc removed their assignment Apr 5, 2017

ppisljar force-pushed the fix/timelionWhitespace branch from a450134 to 15327c8 Compare April 6, 2017 09:42

rashidkpc reviewed Apr 10, 2017

View reviewed changes

tbragin removed the v5.4.0 label Apr 11, 2017

epixa added the release_note:breaking label Apr 11, 2017

ppisljar and others added 8 commits April 24, 2017 07:23

allows more flexible timelion queries

840f50d

removing semi colon, adding support for new lines

a7bdaef

Allow for newlines

720ee94

This make commas requires between arguments and series, but optional at end of either. It would be simple to drop the ','? at the end of each of `arg_list` and `series` if you wanted to require the commas *not* be present.

adding tests

7f0d92b

updating tests

7193932

using bluebird Promise

cd6b457

Series now returns an array, don't need this +

6810136

fixing tests to only test parseSheet

231b6d4

ppisljar force-pushed the fix/timelionWhitespace branch from 3c6ce80 to 231b6d4 Compare April 24, 2017 05:42

thomasneirynck reviewed Apr 24, 2017

View reviewed changes

adding test for whitespace chars (newline and return)

fee2d6b

rashidkpc approved these changes Apr 27, 2017

View reviewed changes

cjcenizal suggested changes Apr 27, 2017

View reviewed changes

improving readability

068d4bd

ppisljar merged commit a3bfc2b into elastic:master Apr 28, 2017

ppisljar mentioned this pull request May 3, 2017

docs: comma to separate queries in Timelion #11582

Merged

Bargs mentioned this pull request May 10, 2017

Support multiline Timelion queries #11164

Closed

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

aphelionz mentioned this pull request May 7, 2018

Accessibility: Kibana 6.3 Meta Issue #18866

Closed

48 tasks


		const expect = require('chai').expect;

		describe('timelion parse_sheet function', function () {


		group
		= '(' space? grouped:series+ space? ')' functions:function* {

allows more flexible timelion queries #10961

allows more flexible timelion queries #10961

Conversation

ppisljar commented Mar 30, 2017

epixa commented Mar 30, 2017

cjcenizal commented Mar 30, 2017

epixa commented Mar 30, 2017

ppisljar commented Mar 31, 2017 • edited Loading

epixa commented Mar 31, 2017

cjcenizal commented Mar 31, 2017 • edited Loading

cjcenizal commented Mar 31, 2017 • edited Loading

ppisljar commented Mar 31, 2017

rashidkpc commented Mar 31, 2017

rashidkpc commented Mar 31, 2017

rashidkpc commented Mar 31, 2017

ppisljar commented Apr 3, 2017

rashidkpc commented Apr 3, 2017

cjcenizal commented Apr 3, 2017

rashidkpc commented Apr 3, 2017 • edited Loading

ppisljar commented Apr 6, 2017

epixa commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbragin commented Apr 11, 2017

thomasneirynck commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Apr 26, 2017

rashidkpc commented Apr 27, 2017

rashidkpc commented Apr 27, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented May 1, 2017

ppisljar commented May 3, 2017

ppisljar commented Mar 31, 2017 •

edited

Loading

cjcenizal commented Mar 31, 2017 •

edited

Loading

cjcenizal commented Mar 31, 2017 •

edited

Loading

rashidkpc commented Apr 3, 2017 •

edited

Loading