Support assert in Painless

[pickypg]

Similar to #20263, but as a simplification of the process for throwing an exception. If we do choose to allow assertions, then we should only allow them in the form that supplies a string with them.

assert someBooleanExpression : 'We broke!'

This would make it easier for end users to throw exceptions without having to try to figure out what exception(s) are supported.

[jdconrad]

I'll take a look into adding this early next week. I don't think it should be too difficult.

[nik9000]

Thinking out loud, will this asset be always on like groove's assert or
optional like java's? What would that mean to scripts? It should probably
throw some exception/error that can't be caught be the scripts but is
caught by you so it doesn't take down the node or otherwise be weird. It'd
just be a script error to the rest of elasticsearch?

On Aug 31, 2016 7:57 PM, "Jack Conradson" notifications@github.com wrote:

I'll take a look into adding this early next week. I don't think it should
be too difficult.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#20264 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANLoiiDluada_fpbjN3HIYDjp21Z-_Lks5qlhTsgaJpZM4Jx3lY
.

[jasontedor]

It should probably throw some exception/error that can't be caught be the scripts but is caught by you so it doesn't take down the node or otherwise be weird.

I think that's what PainlessError can be used for.

[jdconrad]

@nik9000 @jasontedor Using PainlessError is probably the correct way to do this. The only reservation I would have is if there is a reason for there to be a difference between something that is user generated versus something that is internal to Painless, but ultimately it would end up being identical code anyway if we made a second version of the error.

[nik9000]

My instinct would be to make a PainlessAssertionError and a PainlessExecutionError (or something) and have them both extend PainlessError. But I wouldn't be surprised if my instincts were way off.

[jdconrad]

@nik9000 I'm good with this solution. Thanks for the suggestion!

[jdconrad]

Working through this -- after speaking with @rjernst ideally we need a way to disable asserts so they end up being no-ops. I can see three ways of addressing options:

1. add something to the beginning of Painless scripts that allows optional options such as <asserts=true,option=value,...> -- this is nice because it means minimal API changes to scripting in ES and easily allows new options to be added easily.
2. Add options for scripts as both Settings and part of the REST calls. More inline with current ES queries; however this is a lot of infrastructure work.
3. Global settings that apply to all scripts.

[nik9000]

Yeah, I agree with the noop thing. Can it be done with the compilation
parameters? I thought that was a thing you'd piped through a while back. I
know those still have to be piped through the API, but some of the
infrastructure is there?

On Sep 7, 2016 7:34 PM, "Jack Conradson" notifications@github.com wrote:

Working through this -- after speaking with @rjernst
https://github.com/rjernst ideally we need a way to disable asserts so
they end up being no-ops. I can see two ways of addressing options:

1. add something to the beginning of Painless scripts that allows optional
   options such as -- this is nice because it means minimal API changes to
   scripting in ES and easily allows new options to be added easily.
2. Add options for scripts as both Settings and part of the REST calls.
   More inline with current ES queries; however this is a lot of
   infrastructure work.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#20264 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANLotg8BxSFsjoWuwgJKrLKFtJ0ZCFwks5qn0n6gaJpZM4Jx3lY
.

[jdconrad]

@nik9000 There is a <String,String> map in compile that we added to be able to remove a custom script engine that wasn't otherwise necessary. There are a couple other issues that need to be solved depending on the route we go. Global versus script-only options, are they all one or the other? (Both even?) And, will there be any issues with caching since if we don't go with option one, something like asserts used versus being a no-op requires a re-compile.

[nik9000]

something like asserts used versus being a no-op requires a re-compile.

Right. That map probably needs to be part of the cache key in addition to any other plumbing we'd have to do if we used the map.

[clintongormley]

I really like the idea of being able to enable/disable asserts per script, eg with the options at the beginning of the script. Typically you want to debug a single script, rather than turning on asserts for all scripts in your cluster.

[nik9000]

I really like the idea of being able to enable/disable asserts per script, eg with the options at the beginning of the script.

Right. I was thinking something like:

GET hockey/_search
{
  "query": {
    "function_score": {
      "script_score": {
        "script": {
          "lang": "painless",
          "inline": "int total = 0; for (int i = 0; i < doc['goals'].length; ++i) { assert doc['goals'][i] < 1000: 'something'; total += doc['goals'][i]; } return total;",
          "options": {
            "assert": true
          }
        }
      }
    }
  }
}

[jdconrad]

I would prefer just adding the options to the beginning of the script because it requires no change to the REST API, and naturally fits the existing cache without modification. This isolates the change to only Painless.

I also understand there's a bit of plumbing for options (as I had done this work), but this was mostly so we could remove the customized Mustache engine and only have one.

[s1monw]

I would prefer just adding the options to the beginning of the script because it requires no change to the REST API, and naturally fits the existing cache without modification. This isolates the change to only Painless.

to me this is all a question of how do we expose this. I had a chat with @jdconrad and @rjernst the other day about asserts and println in painless and there are a couple of concerns mainly around abuse. We are calling a script a gazillion times such that adding the slightest slow component has a huge impact. The design goal of this language was performance and security. I'm 100% convinced we should not make any compromises to this. We don't enable asserts in production code either unless the user specifies -ea to the JVM. We also don't log crazy stuff by default unless we are debugging something.
That said, I really think we need to think about how to expose these slow features. One scenario that I have in mind is to pass a &debug_document={index}{type}/{id} to the high level request and enable logging and assertions based on this:

• we only apply this request to a single document which is what we should do anyway since it doesn't make sense to debug more than one?
• it prevents people from leaving it on in production and experiencing slowness (hard to misuse)
• we would not even cache the compiled script but recompile every time so no need to add anything to the script or the json.
• we would implicitly enable asserts and println logging (I hope with a limited buffer512kbmaybe?)
• we can transport this information with our thread context everywhere such a that we can leverage it else where. I was thinking about switching on logging per request in the future ie. just log all statements that this request hits (context wise) with debug. That would be a cool feature to have down the road.

If we'd do this we can develop these features in isolation, ie add asserts and logging to painless and expose it separately which I'd prefer.

[nik9000]

One scenario that I have in mind is to pass a &debug_document={index}{type}/{id} to the high level request and enable logging and assertions based on this:

This sounds like we'd need to add a new API for every script calling context. I think that dovetails well with the more strict registration of search contexts that came up in #20426 but I'm a bit weary of adding so many APIs.

[clintongormley]

Discussed with @jdconrad, @s1monw , @rjernst, and @nik9000 - current plan is to add a debug flag to whichever APIs make sense. The script would be recompiled on each execution, enabling asserts (and maybe logging), and not cached. The search API would terminate_after 1 document, to ensure that you don't end up with millions of executions.

[jdconrad]

Closing this as Debug.explain can serve this purpose. Discussion around tools for Painless is proceeding in other issues.