Add new augmentLogEntry property

[mderriey]

Fixes #147

What

This adds a new optional property called augmentLogEntry which allows consumers to add additional properties to the "GraphQL operation" log entry emitted by the logging plugin.

Why

We have a project where we compute a cost associated with each GraphQL operation and want this to be logged.

At the moment, we emit a separate log entry that contains only this information.
It would make sense to log this with the "GraphQL operation" log entry.

Tests

I'm not sure how we could set up automated tests for this, so I installed the built version locally in our project and made the following changes:

graphqlOperationLoggingPlugin<GraphQLContext>({
  contextCreationFailureLogger: logger,
  ignoreIntrospectionQueries: true,
  logLevel: 'info',
  adjustVariables: (variables) => redactSensitiveVariables(variables),
  shouldIgnore: (ctx) => {
    return ctx.source === frontDoorHealthProbeQuery
  },
+  augmentLogEntry: (ctx) => {
+   const { operationCost } = ctx
+   return {
+     operationCost,
+   }
+ },
}),

I ran our app locally, made a query, and this is what the Apire dashboard shows:

The one concern is that logGraphQLOperation spreads rest at the end, which gives consumers the ability to override "fixed" properties. Probably not an issue right now.

https://github.com/MakerXStudio/graphql-core/blob/532c8d9559fd359aab524287bcc980247858c94a/src/logging.ts#L79-L109

Security advisories

The initial PR workflow run had a bunch of issues, which I resolved with:

1. npm update --save, and
2. npm install --save-dev rollup@latest; I'm not sure if rollup being pinned was explicit or not, I took it back to a ^ version constraint.

[cuzzlor]

LGTM Mick. Re your comment, should the additional log props be spread at the start of the object so they cannot override the fixed props?

[cuzzlor]

I'm not sure if you're up agains the same general vulnerabilities as I was.

I decided to try updating ts-toolkit config and all packages, which worked and was overall probably easier than addressing piecemeal, e.g.:

• run ncu to update everything
• run npx @makerx/ts-toolkit init --existing-file-behaviour overwrite
• reinstall one package to re-order package.json
• delete the old eslint config files
• ditch any unwanted overwrite changes to the ts-toolkit config file

[mderriey]

LGTM Mick. Re your comment, should the additional log props be spread at the start of the object so they cannot override the fixed props?

I realised that ultimately it's logGraphQLOperation which has control over the order in which properties are assigned.

The one concern is that logGraphQLOperation spreads rest at the end, which gives consumers the ability to override "fixed" properties. Probably not an issue right now.

https://github.com/MakerXStudio/graphql-core/blob/532c8d9559fd359aab524287bcc980247858c94a/src/logging.ts#L79-L109

Do you think we should update logGraphQLOperation so it spreads rest first, ensuring that the properties we expect to log are always there with the values we expect?

[mderriey]

I'm not sure if you're up agains the same general vulnerabilities as I was.

I decided to try updating ts-toolkit config and all packages, which worked and was overall probably easier than addressing piecemeal, e.g.:

• run ncu to update everything
• run npx @makerx/ts-toolkit init --existing-file-behaviour overwrite
• reinstall one package to re-order package.json
• delete the old eslint config files
• ditch any unwanted overwrite changes to the ts-toolkit config file

Let me try that.

[cuzzlor]

Do you think we should update logGraphQLOperation so it spreads rest first, ensuring that the properties we expect to log are always there with the values we expect?

Oh yeah sorry I forgot that was in another package. Lols at the abstractions, I thought we might use more than just ApolloServer.

Such a minor thing, not worth worrying about, twas my doing anyway :)

[mderriey]

@cuzzlor I think I got there, but it tool me too many attempts for me to have confidence to merge this as-is.

Would you be able to have another look?

I ran npm run build on both main and this branch, and the output looked similar.

[cuzzlor]

Thanks @mderriey - I had a look at the build output locally and it's looking good. I'm gonna merge for you.