[panw] Prefer set with copy_from and ignore_empty_value

[joegallo]

Proposed commit message

Prefer the ignore_empty_value option of the set processor for the panos threat and traffic pipelines, and prefer the copy_from option of the set processor for the panos pipeline.

The logic applies elsewhere, too, but I'm limiting the scope of this change as a proof-of-concept.

ignore_failure operates by catching and ignoring whatever exception(s) might be thrown by the tagged processor, so the (somewhat unfortunate) default behavior of the set processor to throw an exception on an absent value ends up being surprisingly expensive. By way of contrast, ignore_empty_value does what it says, it just ignores the empty value and moves on. So the latter is faster and the former is slower.

The copy_from change was originally part of #15279, but I've pulled it into this PR in order to have all my panw changes in a single place. As with the ignore_failure change, this is a performance optimization: when the field to be copied is already just a string, and so the set processor with mustache isn't being used for the side effect of converting to a string, then it's quite a bit faster to use copy_from rather than value (with mustache templating).

This doesn't matter much for a low volume integration, or one that only uses a handful of set processors, but with many set processors being executed many times it can start to add up.

[taylor-swanson]

@joegallo, this looks great!

I've actually been working on tool for helping make bulk changes to integrations (eventually will make its way into elastic-package, but working on some yaml AST modification stuff in the mean time), and something like this would be an easy and quick addition.

I'll take a closer look at this PR and let you know if anything needs adjusting.

Edit: Also worth mentioning that we are also going to take a look at our use of ignore_failure as pretty much every use of this is wrong as far as I can tell (prefer using something better like ignore_empty_value or conditions where it makes sense).

[taylor-swanson]

Code changes seem fine, we just need to add an entry in the changelog and bump the version in the manifest. If you have elastic-package installed, you can use the changelog subcommand:

elastic-package changelog add --next patch --type enhancement --description 'Prefer set with ignore_empty_value.' --link 'https://github.com/elastic/integrations/pull/15800'

[taylor-swanson]

While we are on this topic... There are also usages like this:

set:
  field: user.name
  value: "{{{destination.user.name}}}"
  ignore_failure: true
  if: ctx.user?.name == null

Any reasons why we'd use templates over copy_from? Same question applies to override vs. field == null.
I can't think of any.

set:
  field: user.name
  copy_from: destination.user.name
  ignore_empty_value: true
  override: false

[joegallo]

Any reasons why we'd use templates over copy_from?

Not for the ordinary case! See #15279 for some of my thoughts on that.

[joegallo]

Same question applies to override vs. field == null.

Ooh, yeah, I don't specifically have a pre-built opinion on that one yet! Naively I'd guess that in the current implementation of things, the if: condition would be faster ('for reasons') but I also think a sufficiently motivated Elasticsearch pull request could close that gap.

Is there an integration that you're aware of that does that a whole lot?

[taylor-swanson]

Ooh, yeah, I don't specifically have a pre-built opinion on that one yet! Naively I'd guess that in the current implementation of things, the if: condition would be faster ('for reasons') but I also think a sufficiently motivated Elasticsearch pull request could close that gap.

Is there an integration that you're aware of that does that a whole lot?

I think we use it quite extensively in our integrations. Probably not for any particular reason other than someone did it in the past and everyone else followed suit. I could see why if would be faster, but from a readability/maintainability standpoint, I like override as it eliminates the possibility of getting the field path wrong (especially when copy/paste is used). I think it makes more sense when you have to do more complicated checks, but most of the time, our use of conditionals is making sure that either the source or destination field isn't or is null (which ignore_empty_value and override seem to handle well).

I also think a sufficiently motivated Elasticsearch pull request could close that gap.

Putting this on my wish list 😄

[joegallo]

To get extremely pedantic, though -- my change here (and in the next stanza) could actually be a change in behavior.

The prior logic with the if: script would not copy_from a null value, but it would copy_from an empty string. The new logic with ignore_empty_value will ignore nulls and it will also ignore empty strings.

I don't imagine that that difference in behavior actually matters, but I don't want to let it sneak by without comment/consideration.

[taylor-swanson]

I don't think that should matter here, thankfully. _conf.external_zones and _conf.internal_zones is set by the integration config to always be either null or an array of strings.

To be honest, I'm not really sure why these processors even exist (since we could just use the conf fields directly), but that's a topic for a different day 😄.

[joegallo]

/test benchmark fullreport

[elastic-vault-github-plugin-prod]

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 03228a8

History

• 💚 Build #33462 succeeded 561744a
• 💚 Build #33371 succeeded 561744a

[elasticmachine]

Pinging @elastic/integration-experience (Team:Integration-Experience)

[joegallo]

The current benchmark report #15800 (comment) is showing as blank (🤷!) but prior to my most recent commit here (4adb496) that should make things even faster, it was showing this:

[taylor-swanson]

LGTM!

[taylor-swanson]

I don't think that should matter here, thankfully. _conf.external_zones and _conf.internal_zones is set by the integration config to always be either null or an array of strings.

To be honest, I'm not really sure why these processors even exist (since we could just use the conf fields directly), but that's a topic for a different day 😄.