feature: Adds common transport for Client Awareness and Enhanced Client Awareness metadata

[calvincestari]

At the moment we have a split between two closely related features; the Client Awareness feature is activated through HTTP headers, whereas Enhanced Client Awareness is activated through request.extensions. The long-term goal for these two has always been to enable a common transport for them. So that they can both be sent via HTTP headers, or request.extensions.

This PR enables that:

• The enhanced_client_awareness plugin now looks for client name/version and adds it to the request context and those values get sent off in the metrics report. This cannot be done because of the timing of trace report generation and sending. The request extensions map is only available post-HTTP processing at which point the trace report has already started and the client identified, doing that in the enhanced_client_awareness plugin is too late and breaks trace reports.
• The telemetry plugin now looks for the library name/version, adds it to the request context, and those values get sent off in the metrics report.

---

Checklist

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

• PR description explains the motivation for the change and relevant context for reviewing
• PR description links appropriate GitHub/Jira tickets (creating when necessary)
• Changeset is included for user-facing changes
• Changes are compatible¹
• Documentation² completed
• Performance impact assessed and acceptable
• Metrics and logs are added³ and documented
• Tests added and passing⁴
  • Unit tests
  • Integration tests
  • Manual tests, as necessary

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. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices. ↩

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

[apollo-librarian]

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 05f197fa31a2af92ca3f2058
Build Logs: View logs

[calvincestari]

@apollographql/router-core, @bonnici - would love a review on this PR when you get a free moment please, thank you.

[bonnici]

The changes look good to me, but from memory the reason we went with using request.extensions was to make it hard for people to modify the library name and version. Adding user-configurable options for deciding which header values to use for this seems to go against that goal. But if we've decided that this is the way we want to go, we should also update the docs here: https://www.apollographql.com/docs/graphos/routing/observability/router-telemetry-otel/enabling-telemetry/usage-guides/client-id-enforcement

[calvincestari]

The changes look good to me, but from memory the reason we went with using request.extensions was to make it hard for people to modify the library name and version. Adding user-configurable options for deciding which header values to use for this seems to go against that goal.

It wasn't necessarily to make it more difficult but rather that using HTTP headers and being on-by-default would have negative consequences for web applications re. CORS. All clients offer a way to disable sending the library values but use hard-coded values, so the only way for an app to modify the values would be to modify the request after it has been composed.

This change now is to clean up the inefficient split sending methods in use today and support a couple recent use cases.

But if we've decided that this is the way we want to go, we should also update the docs here: https://www.apollographql.com/docs/graphos/routing/observability/router-telemetry-otel/enabling-telemetry/usage-guides/client-id-enforcement

Thanks for the prompt about the docs. I'll get those updated.

[calvincestari]

@bonnici, looking into the documentation now is making me question whether customers that choose to send client name and version via request.extensions would be losing trace data? I say this because the changes I've made ensure that the metrics report gets the values regardless of where they originate from (headers vs. extensions) but I haven't accounted for the missing trace report values if they aren't in the http header.

The section titled "Why enforce client reporting?" lists a few items and I'm not certain of where they are derived from: client field usage; traffic patterns; and operation-level observability. Do you know which of those are derived from traces vs. metrics?

[calvincestari]

Putting this PR back into 'draft' while I update the documentation and figure out the trace report questions..

[bonnici]

I'll need to look into this tomorrow.

[calvincestari]

I'm going to add test for client name/version via request extensions + trace report, so I'll have to get that logic path working too.

Not possible, see description.

[calvincestari]

@bonnici - I've removed the client name/version handling from enhanced_client_awareness plugin so now the only change in this PR is handling library name/version when they are sent as HTTP headers.

It would be great if you could please give this another quick glance over before I merge it - thanks.

[bonnici]

Looks good! I tested a request with client name/version in headers on a router running locally and the name/version got set properly in the trace payload. If you want to check the dev data lake you should see a request for the QueryHeadersOnly operation with the library name and version set.