[Logs UI] Add default refresh interfal on streaming

[jennypavlova]

Closes #150626

Summary

This PR sets the default streaming refresh interval to 5 seconds.

Testing

Go to Logs -> Stream
Click on Stream live button
The refresh interval should be now 5s (not 1m as it was before)

[elasticmachine]

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

[Kerry350]

Thanks for looking at this 🙏

The live streaming mode of the Logs UI sets the refresh interval back to 5 s when initializing instead of using the time service default of 60 s

Looking at the ACs we can simplify this process, and tie it explicitly to the initialisation phase of the state machine.

I can see a few problems with the use of this setDefaultRefreshInterval action:

• The setDefaultRefreshInterval action will overwrite any value supplied in the URL. This is a bug.

• Tying a setDefaultRefreshInterval action to the initialized state will technically work here as the transitions on the events are internal (e.g. target: '.initialized' - with a relative target - which means the entry actions won't refire). However, if these events were to re-enter the initialized state we would potentially overwrite changes the user has made in the UI.

• It's not clear why the setDefaultRefreshInterval action would be tied to the initialized state, rather than the initializing states. By the time we move to the initialized state we want everything to be "ready".

• The action is last in the chain, and we always preserve action order. This means we notify the parent (notifyTimeChanged) of a value that is different to the context by the time setDefaultRefreshInterval is called. This won't lead to bugs now, but could in the future.

The order of precedence for the state is defaults > time filter service > URL but we want to disregard the time filter service for the refresh interval when initializing.

The base level defaults are supplied as initial context here. At this point the interval is correct.

We then want to make sure in the next step in the chain, initialising from the time filter service, we don't overwrite this value with the higher value. This is where the value is sent with it's event.. I would still send across the value as part of the event, so it's complete, but disregard it when the updateTimeContextFromTimeFilterService action alters the context, so keep whether it's paused, but disregard the interval.

Lastly if there's a URL interval set, this has ultimate precedence and will overwrite both.

I think this flow makes more sense, what do you think?

[jennypavlova]

@Kerry350 Thank you very much for the good explanation!

The setDefaultRefreshInterval action will overwrite any value supplied in the URL. This is a bug.

Really good point! I understand your concerns and you are right that we don't want to override an URL value if it's not the default one. I can confirm that it happens when you set an interval after starting the stream then stop the stream and refresh the page ( when I tested the same case without stopping the steam it persisted the new value but then it was not overriding the value probably because it was initialized already)

It's not clear why the setDefaultRefreshInterval action would be tied to the initialized state, rather than the initializing states. By the time we move to the initialized state we want everything to be "ready".

Actually, I wasn't really sure if it was the correct state, I wanted it to be after the default value of 1m is set so we won't reset it back but the order of the actions will make sense in those cases instead of moving into a state where we considered everything "ready" 👍

So if I understand correctly we want to override the value inside the updateTimeContextFromTimeFilterService because of the order of the actions the URL override will happen after.
So to make sure we do that only on initializing it should be for the 'INITIALIZED_FROM_TIME_FILTER_SERVICE' event as on update we don't want to do it, right? Also, I will verify that the URL override won't happen on initializing after we tell the time filter service to use 5s instead of the context value (in case this default value of 1m affects the URL).

[Kerry350]

So if I understand correctly we want to override the value inside the updateTimeContextFromTimeFilterService because of the order of the actions the URL override will happen after.

I would say that what we want to do is subtract the value from the updateTimeContextFromTimeFilterService action handling, rather than overwrite anything. As the initial defaults will have set the value. Actually, overwriting would also be fine.

So to make sure we do that only on initializing it should be for the 'INITIALIZED_FROM_TIME_FILTER_SERVICE' event as on update we don't want to do it, right?

That's right, the INITIALIZED_FROM_TIME_FILTER_SERVICE is the event of interest here, and will only be of relevance through the initialisation state. And you're correct we don't want user updates to be disregarded (e.g. the TIME_FROM_TIME_FILTER_SERVICE_CHANGED event).

Unfortunately the time filter service, regardless of whether one or both values was changed, will send both values. So we probably need to check (on an update, not init) "is the value equal to the default". This led to a lot of issues in the past and I ended up having to add an explicit isTimeTouched to the service, but that isn't emitted as part of the observable. This does cause an issue where we can't really differentiate if it was a user initiated change 🤔

There's some very long and very boring context here from when I worked through this last time with some hooks.

Also, I will verify that the URL override won't happen on initializing after we tell the time filter service to use 5s instead of the context value (in case this default value of 1m affects the URL).

I think with the URL we can just leave it's handling as is, because even if we're reading the defaults (but back from the URL) that's fine.

I try to think of this as layering the context from left to right for initialisation:

• Base defaults (initial context to the machine): correct refresh interval 👍
• Time filter: layers on pause value to the context, ignores value
• URL: layers on both pause and value to the context if they exist

For the URL these values may well just reflect the base defaults (as they'll always be pushed once reaching the initialized state) but that's okay. They are ultimately correct.

I think the changes could look somewhat like this (not tested):

export const updateTimeContextFromTimeFilterService = actions.assign(
  (context: LogStreamQueryContext, event: LogStreamQueryEvent) => {
    if (
      event.type === 'TIME_FROM_TIME_FILTER_SERVICE_CHANGED' ||
      event.type === 'INITIALIZED_FROM_TIME_FILTER_SERVICE'
    ) {
      return {
        ...getTimeFromEvent(context, event),
        refreshInterval:
          event.type === 'TIME_FROM_TIME_FILTER_SERVICE_CHANGED'
            ? event.refreshInterval
            : { ...event.refreshInterval, value: DEFAULT_REFRESH_INTERVAL.value },
      };
    } else {
      return {};
    }
  }
);

Alternatively you could have something like: { ...context.refreshInterval, pause: event.refreshInterval.pause } (this defers to the initial defaults, and I think it communicates intent better).

The remaining issue we have is just the "has the user actually touched the interval value, or are we just receiving the default back again". To truly fix that I think we'd need to have the observable of the service communicate that, so that we can do something like && event.refreshInterval.hasBeenTouched.value. Just checking if it matches the default of the service doesn't work, because the user might just choose the same value 🙈 (honestly I'm getting flashbacks to trying to fix this before haha). This is an edge case though, maybe we should ignore this for now.

[Kerry350]

(Just realised you've pushed almost the same change whilst I was writing my comment, sorry 🙈)

[Kerry350]

For context for others:

@jennypavlova and I jumped on a call, and I'm actually an idiot and forgot that, due to the fact we sync our fully initialised values back to the time filter service, we are therefore protected against the issue outlined above. Because we'll just receive our value back essentially, unless the user changes it.

This is nice because it actually validates our ordering and syncing. The time issue was a bit different as the order of precedence was actually a bit different and we had a slightly different approach.

[jennypavlova]

@Kerry350 Thank you for checking that with me, I applied the changes we discussed.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: f6caa48

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
infra	1.3MB	1.3MB	+102.0B

History

• 💚 Build #109604 succeeded 6a7519a
• 💛 Build #109222 was flaky 208ec93

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jennypavlova

[Kerry350]

LGTM 🙌

Thank you for persevering with my stream of thoughts 🙏