Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Correctly consuming core observables requires knowledge of the internal implementation #38397

Closed
rudolf opened this issue Jun 7, 2019 · 6 comments
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@rudolf
Copy link
Contributor

rudolf commented Jun 7, 2019

Context

Most of the data in the Core API is exposed as Observables. Many of these observables aren't a sequence of events, but instead represent the latest value/state. These value observables are implemented as shared/multicast replay subjects. As such, consumers of these observables are usually interested in the first emitted value (and may subscribe to future values). In contrast, in consuming an event observable consumers typically care about the whole sequence of values or the latest.

const status$ = from(['init', 'setup', 'start', 'stop']); // hypothetical event stream

const currentStatus = await status$.pipe(last()).toPromise();
const currentAdminClient = await core.elasticsearch.adminClient$.pipe(first()).toPromise();

Problem

When consuming Core API's it's not clear whether you're consuming a replay/value stream or an event stream and the only way to be sure is to read through the internal code.

See: #35429 (comment)

@rudolf
Copy link
Contributor Author

rudolf commented Jun 7, 2019

Suggested solution:

  1. Use the latest... naming convention for value observables, e.g. latestConfig$ latestAdminClient$
  2. Clearly indicate in the API docs that these observables are replay subjects which will always emit the latest value when subscribed to.

@rudolf rudolf added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jun 7, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@epixa
Copy link
Contributor

epixa commented Jun 7, 2019

The intention here is that plugin developers must take care to stay up to date with the changing nature of this state. I don’t see any value in ever accessing the first value vs the last value for these contracts, and I suspect each occurrence of that is a bug.

Plugin developers should never feel the need to toPromise these things, and if they do there is either legacy baggage or a bug.

This is why the handler interface is so critical, it is the necessary piece to ensure folks don’t need to toPromise their observables.

@kobelb
Copy link
Contributor

kobelb commented Jun 7, 2019

Ignoring the .toPromise, there's a discrepancy between what first returns for the two following examples:

https://codesandbox.io/s/competent-hermann-mleks

import { BehaviorSubject } from "rxjs";
import { first } from "rxjs/operators";

const config$ = new BehaviorSubject(1);
config$.next(2);
config$.next(3);
const first$ = config$.pipe(first(value => value));
first$.subscribe(x => console.log(x));

// outputs
// 3

https://codesandbox.io/s/fast-frog-vb68f

import { of } from "rxjs";
import { first } from "rxjs/operators";

const config$ = of(1, 2, 3);
const first$ = config$.pipe(first(value => of(value)));
first$.subscribe(x => console.log(x));

// outputs
// 1

I do think there are valid scenarios where breaking out of the reactive model and using the imperative model simplifies thing, primarily in the context of HTTP request handling.

HTTP request handling is generally rather short-lived and a lot of times makes persistent changes to some data-store, like Elasticsearch. If we were to try to program a route handler using the reactive model and one of the dependencies, like the config, changes it becomes complicated to determine what should actually be done. If we're using idempotent operations when interacting with the data-store, we could just cancel the route handling logic part-way through and retry the idempotent operations. However, this only really works with idempotent operations, so doing something like a "insert" isn't really possible, we end up always having to do an "upsert". Which makes me wonder, do we really want to be using the reactive model when writing route handlers? Shouldn't using the most recent instance of the config for the duration of the route handling logic be "good enough"? Do we really want developers to have the cognitive overhead of thinking about cancellation for every route handler to prevent them from potentially using a config value which would have been valid a few milliseconds earlier?

@epixa
Copy link
Contributor

epixa commented Jun 10, 2019

@kobelb Contextual handlers are the solution to the problem you just described. I agree that we want a persistent model of our dependencies in the context of each individual route handler. I think when we adopt and roll out that pattern broadly, we'll end up exposing significantly fewer observables to plugin lifecycle functions.

@rudolf
Copy link
Contributor Author

rudolf commented May 5, 2020

Closing in favour of #65224

@rudolf rudolf closed this as completed May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants