refactor: Graph Manager (Engine) reporting "extensions" become "plugins".

[abernix]

This PR depends on #3988 and #3996 (this PR is targeted to merge into the latter). It is part of an initiative to deprecate all internal usages of the graphql-extensions API. Similar work can be seen in the work done to migrate the apollo-tracing extension (#3991) and the apollo-cache-control extention (#3997). Similar to those commits, this introduces:

1. A plugin utilized by the agent's newExtension method which generates a plugin wired up to the mechanism which transmit metrics to Apollo Graph Manager ingress. This plugin is meant to replicate the behavior of the EngineReportingExtension class.
2. A federatedPlugin (i.e. exported on the main module of the apollo-engine-reporting package), meant to replicate the behavior of the EngineFederatedTracingExtension class. This plugin provides the functionality currently used by implementing services in a federated configuration to include the Base64-encoded protobuf trace on the ftv1 extension of responses from the implementing service. This encoded extension data is consumed by the Apollo Gateway when received from an implementing service.

Like the other PRs above, the delta of the commits seemed more confusing by allowing a natural diff to be made of it, so I've left the extensions in place alongside the new plugins in the first commit so they can be compared - presumably side-by-side in an editor - on the same commit. A subsequent commit in this PR removes the existing extensions, so you can choose whether you'd like to view it in its totality or commit by commit.

[glasser]

Sorry for the delay. Targeting tomorrow, high priority.

[glasser]

Ugh, I really meant to get to this today, but time is a slippery thing.

[glasser]

Really glad you're doing this! And thanks for making this so reviewable.

[glasser]

Other than noting the possibly unintentional possibly irrelevant change around logger this looks good to me.

[abernix]

@glasser Could you zoom me back into the specifics of the logger change you're referring to? Certainly seems irrelevant, but it would seem unexpected for it to have piqued your interest and should be unrelated/inadvertent unless it was a result of an out of date merge.

[glasser]

There is a logger in EngineReportingOptions that is used for a variety of things inside agent.ts. So you can theoretically override this logger to some implementation you want used for only GM-reporting stuff.

TreeBuilder also takes a logger that is used for one warning about one edge case relating to errors of the wrong type (without 'path's). Maybe even never happens? Maybe this logger was used for more in the past?

That was the only (remaining?) use of this logger within the request-specific extension.ts.

When you changed extension.ts to plugin.ts, you do this:

    requestDidStart({
      logger: requestLogger,
      schemaHash,
      metrics,
      request: { http, variables },
    }) {
      const treeBuilder: EngineReportingTreeBuilder = new EngineReportingTreeBuilder(
        {
          rewriteError: options.rewriteError,
          logger: requestLogger || logger,
        },
      );

(logger here is the logger from EngineReportingOptions)

So the effect here is that the TreeBuilder's warning now uses the request-specific logger instead of the engine-reporting-specific logger. While it generally makes sense to use the "most specific" logger for any given logging, it's not cut and dry which of those is "more specific" since one has a specific lifecycle whereas the other has a specific subsystem scope.

I think I would lean towards keeping the old behavior. ie, just ignore the logger passed to requestDidStart. But again, it only affects one probably-shouldn't-happen-much warning?

[abernix]

Thanks for the clarification, @glasser, and I missed this and didn't see de7ba72#r416714696 somehow. 🤷

I recall this now and it was a conscious decision when I recently introduced the logger in #3894. Specifically, I avoided introducing the more specific request logger to the graphql-extensions API on the premise that I would soon deprecate it.

I agree that it's less cut and dry than ideal, though the intent of the request-level logger was to capture anything that could have sandbagged a specific request, so in my mind the spirit of using the request logger here is correct, particularly since it could be linked to a specific operation in the same scoped-logger.

On the notion that it only affects the one probably-shouldn't-happen-much (I've never seen, or seen reported) warning, and additionally because we don't specify a way to override the request-level logger right now without specific assignment within a plugin, and the relative recency of this, I'm going to leave this as is for now.

If you have strong feelings about this, I'm happy to change it.

[glasser]

Nope, just wanted to make sure it was understood.