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

[PRFC] Introduce the backend-common Context #8042

Merged
merged 6 commits into from
Feb 2, 2022
Merged

Conversation

freben
Copy link
Member

@freben freben commented Nov 14, 2021

Summary

Introduce a Context type and implementation class, that can be used for explicitly passing contextual information (request / caller context data, deadline / timeout / cancellation, etc) across boundaries in the backend.

Problem Statement

There are cases where you may want to pass contextual information between the different layers of the backend. Some examples are:

  • Incoming service requests may carry some information, such as tracing span IDs, authorization tokens / caller identity, and similar. This data may be relevant to carry along down the stack to be used for similar decoration of upstream network requests.
  • During the request cycle, you may want to enforce some form of deadlines both on the overall fulfillment of that request, as well as the individual parts that make up that processing. This requires communicating some form of timing information down the stack. To illustrate, you may have allotted five seconds until sending a timeout error to the caller of your service handler, and 200 milliseconds per outgoing call that you yourself make to a cache server. When the service handler times out, it should also abort any in-flight requests that were part of its operation.
  • For structured logging / tracing / metrics, each layer may add more contextual labels that they want to be present on the output. Indeed, the service itself may wish to start out with a certain set of base labels, such as the hostname or process ID.
  • When the service is shutting down, all active processes including both request handlers and long running batch work should be signaled to shut down what they are doing in an orderly fashion.

It is possible to construct bespoke solutions for passing all of these types of information through the stack, and some might be partially covered by using global state. However, it easily leads to bloated and hand-wavy APIs that also need possibly breaking changes when new general such features are added or your needs change.

Proposed Solution

Create a Context interface that handles cancellation and a value map, closely modeled in shape and purpose after the golang Context. It has native facilities for abort and timeout, and a general key-value map upon which all other extended features can be built. Each context is immutable but new derived contexts can be made from them with updated cancellation rules, timeouts, or values.

The intended usage of this type is that the service itself makes a single root Context that is passed around, by convention as a first ctx: Context parameter to functions and methods. Every function can either pass the ctx down unchanged to lower levels where needed, or instead make a modified version of it using its mutation methods.

Example (with the label and token APIs just made up to illustrate the idea):

/*
 * packages/backend/src/index.ts
 */
function main() {
  const ctx = Contexts.root().use(
    Contexts.setLabels({ service: 'backstage' })
  );

  router.get('/songs', async (req, res) => {
    const songs = await songStore.read(
      ctx.use(
        Contexts.setTimeoutDuration(Duration.ofMillis(2000)),
        Contexts.setBackstageTokenFromRequest(req)),
        Contexts.setLabels({ feature: 'songs' }),
      ),
    );
    res.json(songs);
  });
}

/*
 * plugins/song-storage/src/store.ts
 */
export class SongStore {
  async read(ctx: Context): Promise<Song[]> {
    const user = Contexts.getBackstageToken(ctx);
    const labels = Contexts.getLabels(ctx);

    const cached = await this.cacheRead(ctx.use(Contexts.setTimeoutMillis(50)));
    if (cached) {
      this.logger.info('Cache hit for songs', labels);
      return cached;
    }

    return await this.serviceFetch(ctx.use(Contexts.setTimeoutMillis(300)))
      .then(data => this.cacheWrite(ctx.use(Contexts.setTimeoutMillis(50)), data));
  }

  private async cacheRead(ctx: Context): Promise<Song[] | undefined> {
    const value = await memcacheClient.read('songs', ctx.abortSignal);
    return value ? JSON.parse(value) : undefined;
  }

  private async cacheWrite(ctx: Context, data: Song[]): Promise<void> {
    await memcacheClient.write('songs', JSON.stringify(data), ctx.AbortSignal);
  }

  private async serviceFetch(ctx: Context): Promise<Song[]> {
    return await fetch('https://songs.example.net', {
      signal: ctx.abortSignal,
    }).then(r => r.json());
  }
}

Risks and Drawbacks

Since the Context has to be passed explicitly into each layer, it quickly permeates a large number of interfaces. This risks becoming a tedious chore to type out in code and pass on. Also, if you initially do not expect to need it, you may have to make a breaking change to your interface when you or something that you depend on starts to consume it.

The Context class therefore also becomes a very central interface, depended upon by both core and plugin APIs. It risks becoming frozen in time and very hard to evolve. It is therefore imperative that it is kept to a bare minimum, using types that are expected to last.

The context is very general in that you can store essentially any information on it. It is important that this mechanism is not abused essentially as a general variable argument list to the function. The golang Context documentation has clear warnings along those lines, and ours should as well.

Alternatives

In Node, it is possible to use continuation-local storage, using the async_hooks feature. This storage spans across callbacks and promises etc, and would suit the context purpose. However, the async_hooks API has been given the lowest stability rating of 1 (Experimental) ever since Node 8. And even enabling it at all - let alone using it - has very significant negative impact on the performance of the Node VM. Async hook state also arguably is harder to debug and inspect, being a more "hidden" feature, compared to having an explicit Context object to look at.

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2021

🦋 Changeset detected

Latest commit: c35e52c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@backstage/backend-common Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@freben freben force-pushed the freben/backend-context branch 3 times, most recently from 4dde5a3 to 60020e8 Compare November 23, 2021 20:52
@freben freben marked this pull request as ready for review November 23, 2021 21:02
@freben freben requested a review from a team as a code owner November 23, 2021 21:02
@Rugvip Rugvip changed the title Introduce the backend-common Context [PRFC] Introduce the backend-common Context Nov 25, 2021
@freben freben added the rfc Request For Comment(s) label Nov 25, 2021
*
* @public
*/
export interface Context {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a fan of the much more minimal Context interface in https://github.com/backstage/backstage/pull/5312/files#diff-89a2ef91b1efe5c84c025ee2db34aba2f91a665f2cc12856f70e915209a205f1

The trouble with a fat context interface is that we can't add or remove any required methods or parameters without a breaking change, making it a poor place to put utility methods.

For example, the current interface doesn't have a withDeadline method, and we couldn't add that without a breaking change. In that case an optional withDeadline method would be pretty much useless as well, as you would have to always be able to fall back to computing the delta instead. What would instead happen in practice is that me ship it as Contexts.withDeadline, contextWithDeadline or something like that, leaving people wondering why there are two different ways to access things.

I think the Go Context interface is pretty well proven, and translated to TypeScript it would just be something like

interface Context {
  readonly deadline?: Date // I think it's best to avoid luxon in core interfaces
  readonly abortSignal: AbortSignal;
  getValue<T>(key: unknown): T;
}

The only missing piece there is the reason the context was aborted, which is actually being worked on

Copy link
Member Author

@freben freben Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not a huge deal really, but one thing that grinds my gears a bit is the mutation patterns not being chainable. :) I think we mentioned that briefly when making the original context implementation.

callFunction(
  Contexts.withLabels(
    Contexts.withRequestData(
      Contexts.withBlah(ctx, a),
      request
    ),
    { label: 'value' })
  );
);

or alternatively

let derived = Contexts.withBlah(ctx, a);
derived = Contexts.withRequestData(derived, request); // god forbid you accidentally do ctx here instead of derived!
derived = Contexts.withLabels(derived, { label: 'value' });
callFunction(ctx);

so would be nice to have something like the with.

callFunction(
  ctx.with(
    Contexts.withBlah(a),
    Contexts.withRequestData(request),
    Contexts.withLabels({ label: 'value' }),
  ),
  // or maybe this reads better and pairs well with Contexts.getX(ctx)?
  ctx.use(
    Contexts.setBlah(a),
    Contexts.setRequestData(request),
    Contexts.setLabels({ label: 'value' }),
  ),
  // or maybe
  Contexts.extend(ctx, builder -> builder
    .setBlah(a)
    .setRequestData(request)
    .setLabels({ label: 'value' }));
);

The setters would boil down to just handling Context => Context functions anyway.

I'd totally be up for condensing the context down as proposed, in terms of data, to the deadline + abortSignal + getValue though apart from that. I mean we could of course even remove deadline and abortSignal but I think golang does strike a good balance there in terms of having a really minimal essential set of things. And besides, the abort signal being in there, is also related directly to other things (it interacts with manual cancellation, timeouts, AND shutdown mechanisms) so having it "outside" the core while being effectlively 100% essential may not be helpful.

I guess doing it with a Contexts helper or even any small set of some kind of common helpers, means that you shift the migration complexity / risk over to those classes instead. When we want to iterate on those builders and extractors of values, people's producing-side code is going to break just as much, and for those consumers that consume anything more than a deadline, abort signal or value, will break too. But I guess that's likely to be a minority of consumers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to get chainability if we feel it's a thing that we should have would be this

callFunction(
  Contexts.decorate(ctx).withLabels({ label: 'value' }).withRequestData(request).withBlah(a).get()
);

Alternatively even having the returned type of all of the with* methods be Context & ContextDecorators, then you could do

callFunction(
  Contexts.withLabels(ctx, { label: 'value' }).withRequestData(request).withBlah(a)
);

Another pattern:

callFunction(
  decorateContext(ctx).withLabels({ label: 'value' }).withRequestData(request).withBlah(a)
);

Or because I think chaining stuff end up being pretty meh in JS very often, tricky to test etc, perhaps just

callFunction(
  Contexts.wrap(ctx, {
    labels: { label: 'value' },
    requestData: request,
    blah: a,
  })
);

Biggest difference here is that it is much easier to evolve an interface that is only consumed, rather than one that is both produced and consumed by user code

@Xantier
Copy link
Collaborator

Xantier commented Nov 27, 2021

I have to say I'm not quite sure on the approach of adding explicit ctx as the first argument to downstream methods. There are few cases in the core packages that this will work because those will be added quickly and would be the default approach in there but when this starts to (slowly) propagate to other managers/services/stores/repositories/clients we will be seeing a lot of bumping happening. Let's say we want to add possible caching to a DB call -> possible semver major. Or if we want to add trace id to a debug log -> breaking change -> possible semver major.

That being said, we are stuck with the ecosystem and without possibility to thread locals it gets very tricky. Using an unstable API as well might be too risky, especially if the side effect of that is an additional perf hit (albeit, I'm not sure if it would be significant?).

Some spitballing on possible alternative approaches:
Could we possibly abuse the JS Proxy object in a way that would give us the ability to decorate arguments without introducing breakages?
Say all items that need access to this ctx would need to be classes, and they would need to extend an abstract class (let's call it Springy ContextAware). This abstract class would proxy all methods on the implementing class and its dependencies (who'd need to be ContextAware as well) by adding traps to getters and applys. The trap handlers would then decorate the arguments with the correct ctx value (which could possibly be held in the abstract class for example).
We might make TypeScript very angry at us with this kind of approach, but I think angry TypeScript could be a lesser tradeoff than semver majors?

I need to codify the idea a bit more to see if that would even be feasible but it could be something to think about..

@Rugvip
Copy link
Member

Rugvip commented Nov 27, 2021

@Xantier Yep the ctx creeping in everywhere is definitely a valid concern. I do see it as a possible solution to a problem that we have but don't have a solution to yet though. For example if you want to introduce an API call to a different plugin backend somewhere and you want to use the user's token, then you're already in breaking change land. I think a typical solution to this in the Node.js ecosystem is to pass on the request object everywhere, but I don't think that's a good pattern to encourage here as we're dealing with an architecture with public APIs quite a couple of layers in, where you really don't want to be dealing directly with the request object. There's also the issue of making calls from non-request contexts like the catalog processing loop.

Tbh, regarding thread local storage, I've never seen that work. Any time I've seen that used in application code it's been a mess and ended up backtracking and removing it. And that's in runtimes that can actually handle it pretty well. As you say it's also experimental in JS.

Proxy object is a bit interesting, especially if we end up with some form of DI where we're already able to decorate implementations. Just like thread local storage I worry it's too much magic and hard to debug, but I think we could explore it a bit. Seems a bit tricky to implement though since JS doesn't have any runtime introspection of parameters other than .toString() xP

@Xantier
Copy link
Collaborator

Xantier commented Nov 27, 2021

Yeah, good points. This whole context thing is something that would be very worthwhile adding, good stuff doing the initial legwork on this @freben.

I'm not certain if proxies will work at all since I haven't looked at them in anger around this area yet. The effort to take a closer look is probably good to go through though, maybe there could be an avenue (or partial avenue) worth taking. There are also concerns around performance on those as well that might come into play.

I like the explicitness of just passing in another param, but fear the surface area that any change will need to touch when this is introduced and ctx param-drilling is put in place. On the other the magic other solutions might introduce will frustrate many developers when trying to figure out what is going on. I'm struggling to see any "good" solutions yet at least around this, so minimizing tradeoffs I guess is the name of the game. :/

@freben
Copy link
Member Author

freben commented Nov 27, 2021

Agreed, this feels like a "find the lesser of all evils" type of thing. It's an unfortunately tricky problem at a high level - introducing cross cutting, layer busting concepts is inevitably tricky, it seems to me.

async_hooks seems like a nice solution at face value, in terms of not having to affect user visible surfaces just to get the propagation problem solved. If we "own" and wrap up its usage, then maybe we could keep our surface stable while the underlying implementation's potential instability is hidden. I also am not sure how important the performance degradation becomes in a nearly 100% I/O bound application like our backends are. It would be great if we could get feedback from community members who have hands-on experience with larger scale use of that feature, to understand tradeoffs better.

@freben
Copy link
Member Author

freben commented Nov 27, 2021

Updated implementation to have the proposed API surface on the Context type, and all "mutation" via Contexts.

Still have not added anything like ContextValueRef<T>, which felt like it perhaps wasn't needed because I hope that the main interaction patterns won't be via direct value(x), but rather via helpers. But that's easy to add if desired.

@freben
Copy link
Member Author

freben commented Nov 27, 2021

@Xantier Let me also highlight that most of the legwork actually comes from @Rugvip in #5312 - It was retracted later though because the timing didn't feel right and we didn't have an active use for it then. So this is a bit of an extension borrowing more from go and leaning on cancelation as a first class thing. Feel free to check out that context variant too, it's actually even leaner than this one!

@freben freben force-pushed the freben/backend-context branch 5 times, most recently from 4525e01 to 1a57c06 Compare November 29, 2021 09:44
@vinzscam
Copy link
Member

I'm trying to understand what are the benefits of the example reported in the description.
In the example above (SongStore) everything is sequential: if the inner Promise rejects, all the rest will also reject. So, what's the point of having a hierarchy of contexts that connect some AbortControllers together?
The only use case I see where this could be used is for dealing with some asynchronous operations going in parallel. But even in this case, the abortController can easily be defined on the fly before await Promise.all([]).

For example, given that all the clients support timeouts as options, the following snippet produce the same results of the one in the description, with less verbosity. Or am I missing something?

/*
 * packages/backend/src/index.ts
 */
function main() {
  const ctx: Context = {
    backstageToken: req.blablabla,
    labels: { feature: 'songs' }
  };

  router.get('/songs', async (req, res) => {
    const songs = await Promise.race([songStore.read(ctx), rejectAfter(2000)]);
    res.json(songs);
  });
}

/*
 * plugins/song-storage/src/store.ts
 */
export class SongStore {
  async read(ctx: Context) {
    const user = ctx.backstageToken;
    const labels = ctx.labels;

    const cached = await this.cacheRead();
    if (cached) {
      this.logger.info('Cache hit for songs', labels);
      return cached;
    }

    return this.serviceFetch()
      .then(data => this.cacheWrite(data));
  }

  private async cacheRead() {
    const value = await memcacheClient.read('songs', { timeout: 50 });
    return value ? JSON.parse(value) : undefined;
  }

  private cacheWrite(data: Song[]) {
    return memcacheClient.write('songs', JSON.stringify(data), { timeout: 50 });
  }

  private serviceFetch() {
    return fetch('https://songs.example.net', {
      timeout: 300,
    }).then(r => r.json());
  }
}

@Rugvip
Copy link
Member

Rugvip commented Nov 30, 2021

@vinzscam the abortSignal part of this pattern is so that we're able to cancel ongoing tasks and avoid unnecessary work. While the promises will resolve correctly, they won't avoid that extra work.

Try for example the following snippet:

function after(ms) { return new Promise(r => setTimeout(r, ms)) }
await Promise.race([
  after(2000).then(() => console.log('done 1') ?? 1),
  after(1000).then(() => console.log('done 2') ?? 2),
])

It'll log done 2 and resolve to 2, but it'll still continue working and eventually print done 1 as well.

This becomes especially important when we can start considering expensive database operations or upstream requests. If the user disappears, someone else replied faster than we could, or we hit the deadline, why continue doing the work? 😁

So looking at your example, let's imagine the songStore.read() takes way longer than expected:

await Promise.race([songStore.read(ctx), rejectAfter(2000)])

We'll of course have the promise rejected after the timeout, but the request will still keep going until we hit some form of timeout, network error, or get a reply. In the case of a late successful reply we'll also go on to JSON.parse, even though it's a waste at that point.

@Rugvip
Copy link
Member

Rugvip commented Nov 30, 2021

I'll add that if what you mean is that we could just set timeouts appropriately, that ends up being a bit of a pain to maintain. You can imagine wanting to change the timeout we allow for a request in one place creating a huge cascade of changes. You'll really end up wanting some good way to communicate deadlines of your async work in that case, which is what the ctx.deadline is for x)

There's not only the timeout use-case to consider though, the race is actually an interesting use-case where we don't really have an explicit timeout. It might just be that whoever replies first wins and we want to cancel the other request(s). That kind of cancellation can't really be handled with timeouts, so you'll end up needing an abort signal or something similar

@vinzscam
Copy link
Member

@vinzscam the abortSignal part of this pattern is so that we're able to cancel ongoing tasks and avoid unnecessary work. While the promises will resolve correctly, they won't avoid that extra work.

Try for example the following snippet:

function after(ms) { return new Promise(r => setTimeout(r, ms)) }
await Promise.race([
  after(2000).then(() => console.log('done 1') ?? 1),
  after(1000).then(() => console.log('done 2') ?? 2),
])

It'll log done 2 and resolve to 2, but it'll still continue working and eventually print done 1 as well.

This becomes especially important when we can start considering expensive database operations or upstream requests. If the user disappears, someone else replied faster than we could, or we hit the deadline, why continue doing the work? 😁

So looking at your example, let's imagine the songStore.read() takes way longer than expected:

await Promise.race([songStore.read(ctx), rejectAfter(2000)])

We'll of course have the promise rejected after the timeout, but the request will still keep going until we hit some form of timeout, network error, or get a reply. In the case of a late successful reply we'll also go on to JSON.parse, even though it's a waste at that point.

sorry but the case you mentioned isn't a real example. There isn't such case in backstage at the moment and I doubt there would be anything similar in the future, where you want two promise run concurrently, letting race between each other...
But still let's consider that. Can't we just define an abort controller right before the race invocation?

const controller = new AbortController();
await Promise.race([
    something({ signal: controller.signal }),
    somethingElse({ signal: controller.signal })
]);

the await will block the execution there, so I don't think there is a need of having a common AbortController defined elsewhere.
But I see the point of cancelling any operation whenever the express' request stream is closed.

@Rugvip
Copy link
Member

Rugvip commented Nov 30, 2021

@vinzscam The timeout example was just to explain the behavior, the songStore.read() is the better more real world example to consider imo.

We can of course manually pass around abortSignals, deadlines, span IDs, user identities, etc. This proposal is about creating a common pattern for that though, and one where the majority of application code doesn't need to be concerned with the details, but simply forward contexts to upstream calls.

@freben
Copy link
Member Author

freben commented Nov 30, 2021

const controller = new AbortController();
await Promise.race([
    something({ signal: controller.signal }),
    somethingElse({ signal: controller.signal })
]);

the await will block the execution there, so I don't think there is a need of having a common AbortController defined elsewhere. But I see the point of cancelling any operation whenever the express' request stream is closed.

You could certainly do that, and as a matter of fact that's effectively what Contexts.setAbort does and can be used for. The difference is that the proposed solution returns a controller that's the union of the surrounding context's abort state and the new inner abort state. It makes sure that if the environment is aborted - for example on service shutdown, on request timeout, or on any number of other unknown factors - then the inner one is also aborted as a side effect. And the exposed deadline is always kept as the minimum (soonest timestamp) of the parent's and the child's desired deadlines.

It is a problem space that is hard to not explicitly solve for in some way. I've encountered several incident scenarios in other services where resources get starved because there is no explicit cancellation - so different pieces of ongoing work stack up infinitely when for example an upstream starts responding too slowly, giving a cascading effect. And we want graceful shutdown to work, etc etc - some form of common solution to these concerns will be needed.

@vinzscam
Copy link
Member

const controller = new AbortController();
await Promise.race([
    something({ signal: controller.signal }),
    somethingElse({ signal: controller.signal })
]);

the await will block the execution there, so I don't think there is a need of having a common AbortController defined elsewhere. But I see the point of cancelling any operation whenever the express' request stream is closed.

You could certainly do that, and as a matter of fact that's effectively what Contexts.setAbort does and can be used for. The difference is that the proposed solution returns a controller that's the union of the surrounding context's abort state and the new inner abort state. It makes sure that if the environment is aborted - for example on service shutdown, on request timeout, or on any number of other unknown factors - then the inner one is also aborted as a side effect. And the exposed deadline is always kept as the minimum (soonest timestamp) of the parent's and the child's desired deadlines.

It is a problem space that is hard to not explicitly solve for in some way. I've encountered several incident scenarios in other services where resources get starved because there is no explicit cancellation - so different pieces of ongoing work stack up infinitely when for example an upstream starts responding too slowly, giving a cascading effect. And we want graceful shutdown to work, etc etc - some form of common solution to these concerns will be needed.

yeah but the example above is not covering that scenario: a just see random timeouts in the code where basically all the abortController are connected together without any specific reason since everything is still sequential. I would scope out this specific "problem".

Regarding the "top to bottom" cancellation, where let's say an express' request connection is closed I totally agree and we have to act on that. My question is: is this the correct approach? Our goal is to FORCE the user thinking about this problem, and as I currently see everything is optional in the fat Context object.

For example the following:

function cacheRead(ctx: Context) { ... }

could become:

function cacheRead(signal: () => any) { ... }

now the signal function is explicitly defined in the client, it would be harder to make mistakes and the user don't need to wasting time digging into documentation, learning about a new "Context" concept used only on this project.

@freben
Copy link
Member Author

freben commented Nov 30, 2021

yeah but the example above is not covering that scenario: a just see random timeouts in the code where basically all the abortController are connected together without any specific reason since everything is still sequential. I would scope out this specific "problem".

Honestly that example may not be awesomely constructed; let's not focus too much on it. I'll see if I can make a more thorough one.

Regarding the "top to bottom" cancellation, where let's say an express' request connection is closed I totally agree and we have to act on that. My question is: is this the correct approach? Our goal is to FORCE the user thinking about this problem, and as I currently see everything is optional in the fat Context object.

Not really seeing how it's fat! It holds the signal that you talk of, plus the deadline that we (and the golang team) considered important to convey in some cases. That can even be passed on to upstream services where needed. The signal is the standard AbortController and AbortSignal out of the standard library (also imported, but 100% compatible with the standard one, just for those that happen to be on very early 14.x versions).

The signal is not optional; the deadline is, if you want to run entirely unbounded. Either way the optionality doesn't affect the typical consumer much since they pass on or read contexts, they don't construct them.

So is the concern about the value part? That's the only thing remaining except the signal and deadline. We do see an actual need to be able to convey user info and auth tokens and similar through the stack without having to modify every intermediate piece of code. How would that be handled? A common thing in express land seems to be to hack on additional fields on the Request object, but I'd argue that that's also a terrible vehicle through all the layers down the stack.

For example the following:

function cacheRead(ctx: Context) { ... }

could become:

function cacheRead(signal: () => any) { ... }

now the signal function is explicitly defined in the client, it would be harder to make mistakes and the user don't need to wasting time digging into documentation, learning about a new "Context" concept used only on this project.

So now consider that the inner code that you in turn call out to, and which is written by another plugin maker at another company, is interested in the deadline to deduce how to best proceed. Oh and also they want to consume the trace ID from the original request to send along.

Not sure how this is less conducive to mistakes either. You generally pass on the parent context if you don't have anything that you want to add to it, which you usually don't.

I think you may be glossing over some of the things that this intends to address. I mean yeah you could make every intermediate function

function cacheRead(signal: AbortSignal, deadline: Date, requestState: Record<string, any>) { ... }
// or
function cacheRead(signal: AbortSignal, deadline: Date, requestState: Request) { ... }

because you aren't sure if the lower layers will need it or not.

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If you are the author and the PR has been closed, feel free to re-open the PR and continue the contribution!

@github-actions github-actions bot added the stale label Dec 22, 2021
@Rugvip Rugvip removed the stale label Dec 23, 2021
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If you are the author and the PR has been closed, feel free to re-open the PR and continue the contribution!

packages/backend-common/api-report.md Outdated Show resolved Hide resolved
export interface Context {
readonly abortSignal: AbortSignal_2;
readonly deadline: Date | undefined;
use(...decorators: ContextDecorator[]): Context;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on dropping this and merging as experimental? 😁
Along with setAbort(src) -> withAbort(ctx, src) etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote to leave them in, mostly based on wanting to start to strongly encourage their use all over the place, with a low discoverability / code size cost at the use site. Feels like the responsible thing to do :D And so easy to pass through anyway that I don't mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave what in? I'm not suggesting removing anything else than use(), it's just that we'd need to tweak the static methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I thought you meant the two above it too, since I didn't see how the set/with thing might come into play otherwise. Hm yeah I could totally go for removing use for now but, then what did you mean by the "Along with" thing? Should we add those setters on the Context interface?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current Contexts.setAbort(src)(ctx) is pretty awkward without use(), so I'm suggesting we make it Contexts.withAbort(ctx, src), and same for the rest of the decorator creators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aight, updated! should all be ready for a second look i think

Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
@freben freben force-pushed the freben/backend-context branch 2 times, most recently from 488dd27 to 90afc6b Compare January 24, 2022 14:56
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
@martina-if
Copy link
Contributor

Hi, I thought I'd leave my feedback on this PR here with the caveat that I'm not an expert in dependency injection or the TS/JS ecosystem, but these are my thoughts:

  • From the list of use cases I think I share a bunch of them, specially around having request data in different parts of the backend and being able to log an opaque user id for each request. I guess for this I'd need to be able to decorate this data with a middleware.
  • I'm not familiar enough with the TS ecosystem to have a well founded opinion but my hunch is that I like this approach, even if it's a little painful in the beginning. As long as these values are treated by the application as optionals, it shouldn't be a huge problem with compatibility, just a little painful to handle, but the benefits are so great that this looks to me like a good enough compromise.

Mammalian Mathematician automation moved this from Review in progress to Reviewer approved Feb 2, 2022
@freben freben merged commit a0f0b8e into master Feb 2, 2022
Mammalian Mathematician automation moved this from Reviewer approved to Done Feb 2, 2022
@freben freben deleted the freben/backend-context branch February 2, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request For Comment(s) will-fix We will fix this at some point
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants