Telemetry enhancements #850

BrynCooke · 2022-04-13T22:08:02Z

🚀 Features

Configurable client identification headers PR #850

The router uses the HTTP headers apollographql-client-name and apollographql-client-version to identify clients in Studio telemetry. Thos headers can now be overriden in the configuration:

telemetry:
  apollo:
    # Header identifying the client name. defaults to apollographql-client-name
    client_name_header: <custom_client_header_name>
    # Header identifying the client version. defaults to apollographql-client-version
    client_version_header: <custom_version_header_name>

🐛 Fixes

Configuration errors on hot-reload are output PR #850

If a configuration file had errors on reload these were silently swallowed. These are now added to the logs.

🛠 Maintenance

End to end integration tests for Jaeger PR #850

Jaeger tracing end to end test including client->router->subgraphs

Router tracing span cleanup PR #850

Spans generated by the Router are now aligned with plugin services.

Review Notes

A new layer has been introduced for instrumentation. It takes a function that returns a span from a request. It is modeled on the tower http instrument layer.
The Jaeger end to end integration test requires Jaeger to be downloaded to a particular location. Not ideal.
Some spans have been migrated directly to the telemetry plugin. This gives us a more consistent view of how long each service takes.

netlify · 2022-04-13T22:08:10Z

✅ Deploy Preview for apollo-router-docs canceled.

Name	Link
🔨 Latest commit	`4b15150`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/6262df799cad5b000849d9fd

NEXT_CHANGELOG.md

apollo-router/src/plugins/telemetry/mod.rs

apollo-router/src/plugins/telemetry/tracing/apollo_telemetry.rs

o0Ignition0o

Wow what a tremendous and super thorough PR 🎉
I ve added a bunch of comments and questions here and there but this looks good to me!

NEXT_CHANGELOG.md

apollo-router-core/src/query_planner/bridge_query_planner.rs

apollo-router/src/lib.rs

apollo-router/src/plugins/telemetry/mod.rs

xtask/src/commands/test.rs

garypen

Mainly looks good and a nice set of cleanup. It seems like the code around the shutting down global tracing has changed since last time I looked at it and I'm not 100% sure it is correct. It will be worth going through that before merging this.

NEXT_CHANGELOG.md

garypen · 2022-04-21T14:33:52Z

apollo-router-core/src/layers/instrument.rs

@@ -0,0 +1,73 @@
+use futures::future::BoxFuture;


After the plugin utils etc stuff merges, you'll see the minimal style of documentation that is required. You can lift some of the documentation from that PR and use it to document the new components (such as this instrument layer/service) that you are adding.

I've added some minimal docs, I'm happy to revisit in a larger docs effort.

apollo-router-core/src/plugins/mod.rs

apollo-router-core/src/query_cache.rs

apollo-router-core/src/services/execution_service.rs

apollo-router/src/plugins/telemetry/mod.rs

apollo-router/tests/common.rs

…module and metrics can go in the metrics module.

Move router_request span to telemetry plugin. There's no way to disable this plugin so it's OK. Add logic for dynamic client name and client version header.

OSX had an issue with max UDP packet size Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572

…d via another thread (which we don't)

… subgraph service.

…y going to get more complicated aw we add more integration tests. Docker compose is the way to go.

Co-authored-by: Gary Pennington <gary@apollographql.com>

Moved instrumentation info span to actual query parsing. It makes no sense to have a span on a cache as the cache hit will mean no time elapsed. Users can now see a span when query parsing happens or no span if parsing did not take place.

…rk to make this test fast enough.

BrynCooke commented Apr 13, 2022

View reviewed changes

NEXT_CHANGELOG.md Outdated Show resolved Hide resolved

BrynCooke linked an issue Apr 14, 2022 that may be closed by this pull request

client identity: allow configuring default header names #426

Closed

BrynCooke self-assigned this Apr 14, 2022

BrynCooke force-pushed the bryn/configurable-client-identification2 branch 8 times, most recently from c2567dc to 36015f6 Compare April 18, 2022 20:41

BrynCooke mentioned this pull request Apr 19, 2022

configurable client identification headers #794

Closed

BrynCooke force-pushed the bryn/configurable-client-identification2 branch 2 times, most recently from 0680b9b to 602506c Compare April 19, 2022 07:28

BrynCooke marked this pull request as ready for review April 19, 2022 07:31

BrynCooke requested review from Geal, o0Ignition0o, garypen and bnjjj and removed request for Geal April 19, 2022 08:36

bnjjj reviewed Apr 19, 2022

View reviewed changes

apollo-router/src/plugins/telemetry/mod.rs Outdated Show resolved Hide resolved

apollo-router/src/plugins/telemetry/tracing/apollo_telemetry.rs Outdated Show resolved Hide resolved

BrynCooke force-pushed the bryn/configurable-client-identification2 branch 6 times, most recently from 152db5b to fc0f735 Compare April 19, 2022 20:50

o0Ignition0o approved these changes Apr 21, 2022

View reviewed changes

BrynCooke force-pushed the bryn/configurable-client-identification2 branch from 2a472b5 to d802a11 Compare April 21, 2022 12:28

BrynCooke changed the title ~~Bryn/configurable client identification2~~ Telemetry enhancements Apr 21, 2022

garypen reviewed Apr 21, 2022

View reviewed changes

bryn and others added 20 commits April 22, 2022 18:19

Add docs public to git ignore.

cc3a570

Move the apollo config, update changelog

00726e8

Split configuration for Apollo so that tracing can go in the tracing …

fcd4c69

…module and metrics can go in the metrics module.

Introduce instrument layer

3e20545

Move router_request span to telemetry plugin. There's no way to disable this plugin so it's OK. Add logic for dynamic client name and client version header.

Print errors if config could not be reloaded.

bec20dc

Update test config files

2dda2aa

Integration test for jaeger

48675e0

Fix build for windows and osx.

94ab1d3

OSX had an issue with max UDP packet size Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572

Lint + take in feedback around operation name

8efc39b

Add logic to kill node processes on test termination

0be78a5

Remove all piping from external commands, it's not safe unless we rea…

055df22

…d via another thread (which we don't)

Revert operation suggestion, operation should be extracted before the…

23ba2f7

… subgraph service.

Take in feedback

93ba22e

Removed instructions for developing without docker compose. It is onl…

28a795a

…y going to get more complicated aw we add more integration tests. Docker compose is the way to go.

Router span name moved to constant.

8113eed

Update NEXT_CHANGELOG.md

68c2c6b

Co-authored-by: Gary Pennington <gary@apollographql.com>

Rename QueryCache#parse_query back to get.

ddca621

Moved instrumentation info span to actual query parsing. It makes no sense to have a span on a cache as the cache hit will mean no time elapsed. Users can now see a span when query parsing happens or no span if parsing did not take place.

Extended integration test to also do hot reloading. Had to do some wo…

ef6b533

…rk to make this test fast enough.

Simplify flush on drop for trace provider.

0f5dd0f

Added some minimal documentation.

bf40b73

BrynCooke force-pushed the bryn/configurable-client-identification2 branch from 1bc9989 to bf40b73 Compare April 22, 2022 16:39

BrynCooke requested a review from garypen April 22, 2022 16:40

bryn added 2 commits April 22, 2022 18:47

Fix compile error after rebase.

5017c68

Fix doc error.

4b15150

BrynCooke merged commit 9a62ca1 into main Apr 22, 2022

BrynCooke deleted the bryn/configurable-client-identification2 branch April 22, 2022 18:07

BrynCooke mentioned this pull request May 4, 2022

release: v0.1.0.preview7 #957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Telemetry enhancements #850

Telemetry enhancements #850

BrynCooke commented Apr 13, 2022 •

edited

Loading

netlify bot commented Apr 13, 2022 •

edited

Loading

o0Ignition0o left a comment

garypen left a comment

garypen Apr 21, 2022

BrynCooke Apr 22, 2022

Telemetry enhancements #850

Telemetry enhancements #850

Conversation

BrynCooke commented Apr 13, 2022 • edited Loading

🚀 Features

Configurable client identification headers PR #850

🐛 Fixes

Configuration errors on hot-reload are output PR #850

🛠 Maintenance

End to end integration tests for Jaeger PR #850

Router tracing span cleanup PR #850

Review Notes

netlify bot commented Apr 13, 2022 • edited Loading

✅ Deploy Preview for apollo-router-docs canceled.

o0Ignition0o left a comment

Choose a reason for hiding this comment

garypen left a comment

Choose a reason for hiding this comment

garypen Apr 21, 2022

Choose a reason for hiding this comment

BrynCooke Apr 22, 2022

Choose a reason for hiding this comment

BrynCooke commented Apr 13, 2022 •

edited

Loading

netlify bot commented Apr 13, 2022 •

edited

Loading