filter/date: Reject invalid UNIX timestamp

[wiibaa]

When using the date filter with UNIX or UNIX_MS format, if an invalid value is given like a not-resolved sprintf %{mytimestamp} , it is evaluated to zero (thanks String#to_i) and thus reset @timestamp to EPOCH time.

Root cause of #1236 and also mentionned in LOGSTASH-1597

Bugfix + spec!

[colinsurprenant]

I do not think it's possible that data be a Numeric here. We are dealing with strings.

[colinsurprenant]

also, is /\d+/ === date too relax? it would match on "a1b" or "a1s2d3" etc, maybe /^\d+$/ ?

[colinsurprenant]

Thanks for you contribution. See inline comments. As noted, this would also solve issue #1236

[wiibaa]

@colinsurprenant I applied you proposal for the code format.
However I kept the numeric check as it was present in the existing spec, I suppose it can come from a grok conversion or a mutate or even a json input.
If everything was really only string it would be too simple ;)

[colinsurprenant]

ok, thanks, will review & test locally shortly.

[wiibaa]

@colinsurprenant I have rebased this PR against master, hoping I did it correctly.

Also it seems that the spec file is not fully consistent on how it calls and test the timestamp value like

Two kind of access to timestamp

insist { subject.timestamp.time } ==
insist { subject["@timestamp"].time } ==

Missing .time in non-equals test

insist { subject["@timestamp"] } !=

it would need a 2nd-pass on the complete file, a little out of scope of this PR

[suyograo]

@wiibaa Can you make this non capturing group? Like /^\d+(?:\.\d+)?$/ Will give it some performance gain

[wiibaa]

@suyograo done

[elasticsearch-release]

Can one of the admins verify this patch?

[suyograo]

LGTM

[colinsurprenant]

@suyograo @wiibaa did we run a benchmark before/after to see the impact of doing a regex check at every parse?

[suyograo]

I'll check, but using non capturing groups was pretty quick. Also we could flip the order of unless /^\d+(?:\.\d+)?$/ === date || date.is_a?(Numeric)

[wiibaa]

@colinsurprenant I duplicated logic in spec/filters/date_performance to get events/sec ratio for UNIX and UNIX_MS parsers with both kind of entry string vs numeric, and executed a few time locally to get an average value for each possibilities. here are the results:

With validation the main reduction in speed is when parsing string, circa -1400evt/sec
With @suyograo proposal to flip the conditions, we mitigate the validation cost for integer value but it increases a little more the string validation cost.

Is it sufficient for you or do you use different solution internally for benchmarking ?

[wiibaa]

@colinsurprenant @suyograo I though more about this, the initial issue is that String#to_i String#to_f never fails but return 0 when the string is not a valid number, an alternative to regex would be to use Integer() and Float() that internally raise an exception

when "UNIX" # unix epoch
  parsers << lambda do |date|
    (Float(date) * 1000).to_i
  end
when "UNIX_MS" # unix epoch in ms
  parsers << lambda do |date|
    Integer(date)
  end

the raised exception would look like:

Failed parsing date from field {:field=>"mydate", :value=>"%{bad_value}", :exception=>#<ArgumentError: invalid value for Float(): "%{bad_value}">,

and the mini-benchmark

so except a small loss for string parsing with UNIX pattern the other cases validation is for free
What do you think?

[colinsurprenant]

I'm a bit confused, isn't the isNumeric approach the fastest?

Also, you might want to look at https://github.com/elasticsearch/logstash/tree/master/test/integration for performing benchmarks with real config and real data.

[wiibaa]

@colinsurprenant in fact I discovered a better approach, according to my testing, after this PR was merged, explaining the monologue after this PR has been closed...
I will try to use your benchmarks and if relevant do a PR direclty on the date filter repo