ES|QL: pin 'now' as query start time

[luigidellaquila]

Use now (already present in ESQL Configuration) to determine the start time of the query.
The serialization is already in place, so the value is already shared between the nodes.

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[astefan]

LGTM

[bpintea]

FTR: https://github.com/elastic/elasticsearch/pull/103474/files#r1427142330

I'm wondering if this method shouldn't be replaced with what it does on the result of now(), at it's sole invocation place in the ComputeService class. (Or, less ideal, maybe rename it to epochNow()?)

[astefan]

My original suggestion was to place the now initialization time in the constructor of the Configuration instead. I don't see why this now value should come from "outside", but in that PR things were considered differently and I didn't object.

[bpintea]

My #112595 (comment) was to place the now initialization time in the constructor

👍
I guess it should work if all these should just use one nanos value, init'ed in c'tor. And I guess we can have a protected c'tor that takes the value too, for testing? Plus a now() method returning zoned time off it.

[luigidellaquila]

My #112595 (comment) was to place the now initialization time in the constructor of the Configuration

Right now the initialization is in the constructor; I think we made the change when we removed EsqlConfiguration

About the "zoned" now, my understanding is that now.instant() will be an absolute value, independent from the zone, so we can rely on it, right?

[bpintea]

the initialization is in the constructor

There are two times now, one init'ed in the c'tor (the old one), the other one passed in, added in the PR Andrei mentioned (a new one). We could/should make just one out of them.

so we can rely on it, right?

That's my understanding too.

[luigidellaquila]

There are two times now

Oh, I was looking at the wrong place... 🤦

I'm not sure I fully understand the difference between absoluteStartedTimeInMillis and queryStartTimeNanos (apart from the precision).
now and absoluteStartedTimeInMillis are practically instantiated at the same time, and now has nanosecond precision, so maybe we can use it for both?

I don't know, I'm confused...

[nik9000]

Seems right.

[nik9000]

I'm not sure I fully understand the difference between absoluteStartedTimeInMillis and queryStartTimeNanos (apart from the precision).

I'm not sure why we have the nanosecond one to be honest.

[nik9000]

I'm not sure why we have the nanosecond one to be honest.

Oh! Sorry one second.

[nik9000]

Oh! Sorry one second.

Ok. I don't see a queryStartTimeNanos, but in general nanosecond precision timers don't measure absolute time. They can't be used for "time since epoch" - they are relative timers. You can think of them as measuring time since process start. Or bootup. Or just some random time in the past.

The other thing is the NTP can change the absolute time. That's sort of it's job - time syncing. Generally NTP will change the absolute timers slowly, but if you are trying to figure out "how much time did this take" then stuff like NTP can get in the way. Sometimes you'll get negative numbers for "took" time if you use the absolute clock. But the relative clock is fine for this.

[luigidellaquila]

stuff like NTP can get in the way

I'd say this is enough to keep now and queryStartTimeNanos separate.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.x

[bpintea]

I'd say this is enough to keep now and queryStartTimeNanos separate.

Since getQueryStartTimeNanos() is later subtracted again from System.nanoTime(), this makes sense.
But having a now() method along with absoluteStartedTimeInMillis(), which just returns the millis in now(), 🤷 .