Add better context config for coprocessor to help migration

[bnjjj]

Context configuration

You can configure what you want to send from the context to your coprocessor. By default it's disabled (false) and doesn't send any context keys to the coprocessor. You now have the ability to give an array
containing all the context keys you want to send to your coprocessor. Sending only a selective list of context keys will increase the performances because it will lower the size of payloads between the router and coprocessor.
You can also set context: deprecated if you still want to use deprecated context key names from router 1.x. If you want to pass all context keys with new naming just set context: all.

Example:

coprocessor:
  url: http://127.0.0.1:3000 # mandatory URL which is the address of the coprocessor
  supergraph:
    request:
      headers: true
      context: # It will only send these 2 context keys to your coprocessor
        selective:
          - apollo::supergraph::operation_name
          - apollo::demand_control::actual_cost
      body: true
    response:
      headers: true
      context: all # It will send all context keys with new names (2.x version)
      body: true
  subgraph:
    all:
      request:
        context: deprecated # It will send all the context keys with deprecated names (1.x version)

---

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

• Changes are compatible¹
• Documentation² completed
• Performance impact assessed and acceptable
• Tests added and passing³
  • Unit Tests
  • Integration Tests
  • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩

2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

[svc-apollo-docs]

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 1 changed, 0 removed

* graphos/reference/upgrade/from-router-v1.mdx

Build ID: 737f6a158aa049d92b86fcd3

URL: https://www.apollographql.com/docs/deploy-preview/737f6a158aa049d92b86fcd3

[github-actions]

@bnjjj, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

[lennyburdette]

👍🏻 to this idea — we added a connectors-specific config option to do something similar: https://github.com/apollographql/router/blob/dev/apollo-router/src/plugins/connectors/configuration.rs#L43

should we remove that option in favor of this approach?

[bnjjj]

@lennyburdette good catch ! I think yes that would probably be enough to just include the context key in the coprocessor config. But for now the default config is to pass all context keys. Could it be an issue if your connector keys are passed by default ?

[lennyburdette]

we add the key only if you set that connectors-specific config value: https://github.com/apollographql/router/blob/dev/apollo-router/src/plugins/connectors/plugin.rs#L147-L175 ... we could switch this logic to read from your new config?

[theJC]

I like this idea, but I would argue that a prerequisite would be for Apollo to supply a compendium/reference of all possible context keys in the Router documentation for your customers, so they know what context keys they can choose from.

A list was added to the docs in docs/source/routing/customization/overview.mdx in this MR: https://github.com/apollographql/router/pull/6572/files#diff-e95018a62736abd100a39b77182d90ffa70565ec35cad5cb0cd366b1535f306a

But then subsequent document updates seemed to have removed it.

[garypen]

General Comment:

I don't like having DEPRECATED const values scattered around the code base. I'd prefer if they were isolated to a single deprecated source file.

I have a question about the selective option. Does it only work with "new" names or can you use "deprecated" names? Also, is it possible to mix and match them within the same array? Would this lead to duplication and confusion?

[bnjjj]

@garypen

I have a question about the selective option. Does it only work with "new" names or can you use "deprecated" names? Also, is it possible to mix and match them within the same array? Would this lead to duplication and confusion?

No it's only new names

[bnjjj]

Good catch @theJC I added back the docs for context keys you linked

[bnjjj]

@lennyburdette if it's an opt-in option that's even better. I think you're right it should check the new configuration for context and add it if it's included in the context keys list

[bnjjj]

@garypen I agree with you about the deprecated context keys. I think it's better to put everything into the same module/file. Can we just consider doing this in a follow up PR ?

[garypen]

@garypen I agree with you about the deprecated context keys. I think it's better to put everything into the same module/file. Can we just consider doing this in a follow up PR ?

It could be done in a follow-up.

[garypen]

@garypen

I have a question about the selective option. Does it only work with "new" names or can you use "deprecated" names? Also, is it possible to mix and match them within the same array? Would this lead to duplication and confusion?

No it's only new names

We should document that clearly.

[bnjjj]

@garypen Added in 05bc550

[goto-bus-stop]

Is/should this also be available for Rhai scripts?

[bnjjj]

@goto-bus-stop Yes but we will do this in a followup PR because it's taking more time. And I think it's also easier to spot in a rhai script when you're using a deprecated key

[theJC]

While focusing on optimizing the performance/efficiency of communicating with the coprocessor, can I please put a bug in your ear regarding looking into the possibility of adding Unix Domain socket support for communicating with coprocessor (#5739)

Potentially better long term might be looking into the viability of refactoring/DRYing as much as possible of the httpclientservice used for outbound calls to subgraphs and the similar/duplicative work done for outbound calls for the coprocessor. That way advances/improvements/changes/observability there are available and leveraged for both pieces of communication (as well as hopefully sharing common configuration)

Not having this consistent/common between these areas causes and will continue to cause additional efforts and asks from your customers to get them aligned as well as duplicative work for you.

Several examples of efforts and asks to get coprocessor calling caught up with subgraph calling capabilities:

• enabling http2/h2c - I contributed this last year - Enable capability of using http2 cleartext (h2c) when communicating with coprocessor #5300
• having common observability - I just sent this MR this week: Instrument coprocessor request with http_request span #6776
• supporting unix domain sockets for coprocessor - a request Indeed made last year External Coprocessor: Support for connections to be made with Unix Domain Sockets #5739