search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.72k stars 8.13k forks source link

[APM] Should this promise be retried? #182274

Open afharo opened 4 months ago

afharo commented 4 months ago

The promises below might fail in case there's a glitch in ES connection and never retry.

https://github.com/elastic/kibana/blob/593d3911728cf68093925836ae2031746c7db6cd/x-pack/plugins/observability_solution/apm/server/lib/apm_telemetry/index.ts#L120-L149

https://github.com/elastic/kibana/blob/593d3911728cf68093925836ae2031746c7db6cd/x-pack/plugins/observability_solution/apm/server/plugin.ts#L71-L79

https://github.com/elastic/kibana/blob/593d3911728cf68093925836ae2031746c7db6cd/x-pack/plugins/observability_solution/apm/server/plugin.ts#L130-L141

Is that OK? Should we apply any retry logic?

If we want to retry, p-retry could be a useful utility that's already present in the project.

elasticmachine commented 4 months ago

Pinging @elastic/apm-ui (Team:APM)

dgieselaar commented 4 months ago

@afharo It's okay to not retry. We want to keep this as unintrusive as possible, to prevent any unnecessary load on the cluster.

afharo commented 4 months ago

@dgieselaar, thanks for confirming! FWIW, I created this issue for awareness. Closing the issue is perfectly valid.

Just to confirm about this case https://github.com/elastic/kibana/blob/593d3911728cf68093925836ae2031746c7db6cd/x-pack/plugins/observability_solution/apm/server/plugin.ts#L130-L141

If the APM promise fails, the plugin won't register the tutorial

Wrt telemetry-related promises, if they fail, we won't have telemetry. But I understand we might be ok with that to avoid overloading the cluster.

dgieselaar commented 4 months ago

@afharo ah sorry missed the last one! I think a retry makes sense there, but I will leave it up to @elastic/obs-ux-infra_services-team to figure out what is best there. Thanks for bringing it up in any case!