refactor: Introduce "tracing" plugin and switch request pipeline to use it. #3991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…se it. This commit introduces a `plugin` (named) export from the `apollo-tracing` package, which uses the new "plugin" hooks rather than the previous implementation (exported as `TracingExtension`) which uses the soon-to-be-deprecated "extensions" hooks. The functionality is intended to be identical, in spirit. Since the delta of the commits was otherwise confusing, I've left the `TracingExtension` present and exported and will remove it in a subsequent commit. Briefly summarizing what the necessary changes were: 1. We no longer use a class instance to house the extension, which was necessitated by the `graphql-extensions` API. This means that uses of `this` have been replaced with function scoped variables by the same name. 2. The logic which actually does the formatting (previously handled by the `format` method in `graphql-extension`, now takes place within the plugin API's `willSendResponse` method.
This follows-up 672fa09 and removes the no-longer used/necessary `TracingExtension` extension which uses the soon-to-be-deprecated `graphql-extensions` API, now that it has been replaced with an implementation that uses the plugin API.
This package should never have already set it itself prior to this point, but the circumstance exists that something (anything) else could have already used the `extensions` sink to attach a value. This guards against that.
abernix
force-pushed
the
abernix/migrate-tracing-to-extension-api
branch
from
5c9ceaa
to
7e4cc4b
Compare
abernix
added a commit
that referenced
this pull request
Apr 16, 2020
Inspired by landing some PRs separately and a merge commit that could have been avoided, but also inspired by the following comment by @trevor-scheer whicih made it clear my organization was just a _bit_ off. Ref: #3991 (comment)
abernix
added a commit
that referenced
this pull request
Apr 16, 2020
abernix
force-pushed
the
abernix/migrate-tracing-to-extension-api
branch
from
c930c24
to
3e9188f
Compare
…ernix/migrate-tracing-to-extension-api
abernix
added a commit
that referenced
this pull request
Apr 16, 2020
…ns". Similar to 6009d8a (#3991) and 68cbc93 (#3997), which migrated the tracing and cache-control extensions to the (newer) request pipeline plugin API, this commit introduces: - Internally, a `plugin` named export which is utilized by the `agent`'s `newExtension` method to provide a plugin which is instrumented to transmit metrics to Apollo Graph Manager. This plugin is meant to replicate the behavior of the `EngineReportingExtension` class which, as of this commit, still lives besides it. - Externally, a `federatedPlugin` exported on the main module of the `apollo-engine-reporting` package. This plugin is meant to replicate the behavior of the `EngineFederatedTracingExtension` class (also exported on the main module) which, again as of this commit, still lives besides it! Again, the delta of the commits seemed more confusing by allowing a natural `diff` to be made of it, I've left the extensions in place so they can be compared - presumably side-by-side in an editor - on the same commit. An (immediate) subsequent commit will remove the extension.
|
While this work (whose review I'll leave up to @trevor-scheer) is worthwhile, can you while you're at it (and as part of describing it in CHANGELOG) update its description/docs to make it clear that it's more or less deprecated? My understanding is that the primary use case of apollo-tracing was for AGM but that only worked via engineproxy; the README does not make it clear that that's a highly deprecated use case. I think there's also some graphql-playground support for it too. I think users get confused a lot into thinking this package (and its enabling flag) has anything to do with modern AGM reporting, but it doesn't. |
…abernix/migrate-tracing-to-extension-api
abernix
commented
Apr 21, 2020
…abernix/migrate-tracing-to-extension-api
This reverts the public messaging about `tracing` deprecation in commit 98bae44, for now, until we can discuss its fate a bit more, and how we intend on making up for that.
…abernix/migrate-tracing-to-extension-api
abernix
force-pushed
the
abernix/migrate-tracing-to-extension-api
branch
from
f7e410e
to
4fbf8a7
Compare
|
I have reverted (ef4ed58) the comment I added to the While I agree with us framing it appropriately and clearly, let's do that ASAP outside the scope of this PR. |
…abernix/migrate-tracing-to-extension-api
…abernix/migrate-tracing-to-extension-api
…rate-tracing-to-extension-api
abernix
changed the base branch from
abernix/add-willResolveField-and-didResolveField
to
release-2.14.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces a
plugin(named) export from theapollo-tracingpackage, which uses the new "plugin" hooks rather than the previous implementation (exported asTracingExtension) which uses the soon-to-be-deprecated "extensions" hooks. The functionality is intended to be identical, in spirit.Since the delta of the commits was otherwise confusing, I've left the
TracingExtensionpresent and exported and will remove it in a subsequent commit.Briefly summarizing what the necessary changes were:
graphql-extensionsAPI. This means that uses ofthishave been replaced with function scoped variables by the same name.formatmethod ingraphql-extension, now takes place within the plugin API'swillSendResponsemethod.Finally, this removes the rendered-unused/unnecessary
TracingExtensionextension after providing its replacement.