[Telemetry] Usage collectors not using isReady correctly

[chrisronline]

The following collectors need to change their isReady to properly wait for the task manager state to exist:

• Actions
• Alerts
• APM APM is unique, as their telemetry data is not something easily computed
• Lens

They are intentionally catching errors where the task manager hasn't ran their task yet, but that should be used to indicate if the usage collector is ready or not.

cc @TinaHeiligers

[elasticmachine]

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

[TinaHeiligers]

Thanks @chrisronline!
Related to #52446

[TinaHeiligers]

@chrisronline the actions, alerts and lens collectors explicitly comment that they're fine with not returning any usage data the first time that their collectors' fetch methods are called. I could still follow up with them with draft "fixes" to double check that they're ok with skipping data.

[chrisronline]

@chrisronline the actions, alerts and lens collectors explicitly comment that they're fine with not returning any usage data the first time that their collectors' fetch methods are called. I could still follow up with them with draft "fixes" to double check that they're ok with skipping data.

Sure, but we don't have to settle for that and I don't know why we would. We have a way to ensure the telemetry data can always be consistent. My guess is that the authors of those collectors aren't aware that a "miss" might mean there is no telemetry data for that user the entire day.

[TinaHeiligers]

My guess is that the authors of those collectors aren't aware that a "miss" might mean there is no telemetry data for that user the entire day.

@chrisonline Gotcha! I'll update the README and change the example code to make it clearer.

[chrisronline]

FWIW, we've had this ability for some time now and there is some history in the original PR: #36153

[TinaHeiligers]

@chrisronline The updated README gives more info on using isReady. Is that sufficient to close this issue or would you prefer to wait on the teams involved?

[chrisronline]

@TinaHeiligers Yea that's great, thanks for adding that!

cc @mikecote for any thoughts on if the alerting team should update the alerts/actions collectors

[TinaHeiligers]

cc @wylieconlon Lens
cc @dgieselaar APM

You might want to reconsider the implementation of isReady in these collectors.

[dgieselaar]

@TinaHeiligers this is intentional - our telemetry collection can take a long time (minutes). The advice from the telemetry team (IIRC) was to not block telemetry collection on startup. Is the recommendation now different?

[chrisronline]

The telemetry collectors will wait up to 1 minute before attempting to send usage data without that collector; however, AFAIK, that time period is somewhat arbitrary and possibly changeable.

@dgieselaar Just curious, can you provide more information on why it takes so long?

[mikecote]

I created this issue #82947 for the alerting team to explore / fix this.

[dgieselaar]

@chrisronline heavy queries (aggregations on billions of documents), and we run them sequentially to be on the safe side as to not overloading the cluster.

[chrisronline]

Thanks @dgieselaar.

Once this data comes back, is it persisted in memory so that the next telemetry collection cycle picks it up? If so, does it persist in memory until Kibana is restarted? Is it ever re-fetched?

[afharo]

@chrisronline AFAIK, they use taskManager to periodically run those queries and they store the results in a savedObject. Then they retrieve the results from the SOs in the fetch method. Maybe @dgieselaar can confirm. If that's the behaviour, I think it is safe to restarts 🙂

[dgieselaar]

That's correct @afharo, thank you - and apologies to @chrisronline for missing this earlier :). We run our queries every 12hrs.

[chrisronline]

I see, okay. It seems we need to make an exception for APM. I'll update the issue

[TinaHeiligers]

@wylieconlon Have you had a chance to review the Lens usage collector's implementation of isReady yet?
cc @flash1293

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[flash1293]

@TinaHeiligers I think this fell through the cracks, is there a deadline for this?

[TinaHeiligers]

@flash1293 There isn't a strict deadline from the Core team as such, since it's up to plugin owners to decide how important their data is for them. As with most things telemetry-related, the sooner we ensure we're capturing consistent data, the better 😄

[flash1293]

I checked the code and it seems like we have the same structure as other consumers listed (although implemented a little different)

@gmmorris Seems like you implemented this, could you confirm it doesn't matter whether isReady is async itself (like here https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/usage/actions_usage_collector.ts#L33-L36 ) or returning false synchronously until it's ready (like here https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/server/usage/collectors.ts#L19-L23 )?

[YulNaumenko]

I checked the code and it seems like we have the same structure as other consumers listed (although implemented a little different)

@gmmorris Seems like you implemented this, could you confirm it doesn't matter whether isReady is async itself (like here https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/usage/actions_usage_collector.ts#L33-L36 ) or returning false synchronously until it's ready (like here https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/server/usage/collectors.ts#L19-L23 )?

@flash1293 you are right it doesn't matter as the approach, because you are doing the same check if the Task Manager is ready or not and isReady method support both boolean and Promise<boolean>. But in your case for keeping the code cleaner and straightforward the better usage of isReady will be to have it async, because you have the your check as async.