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

ingest: extended if documentation #35044

Merged
merged 7 commits into from
Nov 26, 2018

Conversation

jakelandis
Copy link
Contributor

part of #33188

@acchen97 - requesting your review since the if conditional enables new ingest patterns and wanted to get your feedback here. Let me know if you want me to upload the rendered doc somewhere.

@jakelandis jakelandis added >docs General docs changes :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.0.0 v6.5.0 labels Oct 29, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jakelandis jakelandis changed the title Doc extended conditional ingest: extended if documentation Oct 29, 2018
@jakelandis
Copy link
Contributor Author

cc @jdconrad since this is partly an introduction to using Painless

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

Left a few remarks :) Generally looks fine (and very useful for users). I'm just a little worried, we're now using the conditionals docs to make up for shortcomings in the Painless docs? (this is really good stuff though, maybe it should just be moved to the painless docs where it gets too general?)

to `true` or `false`.

For example the following processor will <<drop-processor,drop>> the document
(e.g. not index it) if the input document has a field named `network_name`
Copy link
Member

Choose a reason for hiding this comment

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

e.g. -> i.e. I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks. (i always mix up those two)

level `b` object.

To help protect against NullPointerExceptions, null safe operations should be used.
Fortunately Painless makes {painless}/painless-operators-reference.html#null-safe-operator[null safe]
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs a comma right? Fortunately,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// TESTRESPONSE
////

The source document can also use dot delimited fields to represent nested fields.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so sure about this section. It's a nice explanation of the dot expander processor, but it seems to get a little off-topic? (it just seems to be a very specific example and I don't see how it's any different from explaining that you could use another script processor before your if to do a transformation on your doc before evaluation the conditional)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The section is titled Handling Nested Fields in Conditionals and wanted to include this since it is (imo) not obvious. Admittedly this ended up more of handling null references section and it is abit out of place.

I don't have strong opinions of keeping it or tossing it, but lets see @acchen97 says about it's usefulness. If input documents are rarely in dot syntax then we should toss this, if it is common we can discuss how to integrate it better.

running in the {painless}/painless-ingest-processor-context.html#null-safe-operator[ingest processor context]
can be used to determine if the condition should evaluate to `true` or `false`.

IMPORTANT: Values in an `if` condition are read-only and may not change any
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Wouldn't just: "The value of ctx is read-only in if conditions. Trying to mutate it will throw and Exception." be cleaner? (because it's not "values" that are read-only but just ctx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, i like your suggestion... updated.

IMPORTANT: Values in an `if` condition are read-only and may not change any
values of `ctx`.

A more complex `if` condition that drops the document (e.g. not index it)
Copy link
Member

Choose a reason for hiding this comment

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

e.g. => i.e. again right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

--------------------------------------------------
// CONSOLE

The conditional needs to be all on one line since JSON does not
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be explained elsewhere. This is a very general fact about scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i really wanted the if conditional (script) to be readable on the page which requires the triple quote, but only wanted to provide a CONSOLE example that is valid JSON. Not sure how to achieve readability and valid JSON without having to repeat this fact.

@jdconrad
Copy link
Contributor

jdconrad commented Nov 1, 2018

The Painless scripts look fine to me. Thanks for the ping.

@jakelandis jakelandis removed the request for review from acchen97 November 5, 2018 20:21
Copy link

@kvch kvch left a comment

Choose a reason for hiding this comment

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

Extensive documentation which covers everything from the simplest to more complex use cases. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >docs General docs changes v6.5.0 v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants