Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(graphql-server): Conditionally enable OTel plugin and OTel plugin updates #8782

Merged
merged 9 commits into from
Jun 30, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

Problem

  1. The redwood OTel plugin was being enabled automatically rather than being opt-in. This causes issues for users who want to use a different OpenTelemetry setup.
  2. It was not possible to pass options to the OTel plugin.

Changes

  1. The redwood OTel plugin is now only enabled when the openTelemetryOptions property is passed in by the user where they create their graphql server/handler.
  2. The openTelemetryOptions provides a mechanism to provide the 3 options that the existing envelop plugin has. (see https://github.com/n1ru4l/envelop/tree/main/packages/plugins/opentelemetry)
  3. The OTel plugin now starts active spans using startActiveSpan
  4. The setup command now requires the user to add this openTelemetryOptions property manually and so the messaging has been updated. (I will also update the forum post)

Fixes
Fixes #8332

Notes
@pvenable - This is long overdue so sorry about that! I have included the three standard options via this new openTelemetryOptions property which you would use as shown below. I also used startActiveSpan and I believe this would suit your needs although I have no test project with the particular setup you mentioned.

I would be very, very grateful if you could try this out when it makes it into a release and feedback again on your experience. It would be good if we could make redwood do what you need out of the box!

export const handler = createGraphQLHandler({
  authDecoder,
  getCurrentUser,
  loggerConfig: { logger, options: {} },
  directives,
  sdls,
  services,
  onException: () => {
    // Disconnect from your database with an unhandled exception.
    db.$disconnect()
  },
  openTelemetryOptions: {
    resolvers: true,
    result: true,
    variables: true,
  }
})

@dthyresson - This did not change the behaviour we have before where the results of makeMergedSchema were being reported directly under the query span rather than under any particular resolver. I continue to question this but I think we were happy with it before.

@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Jun 29, 2023
@Josh-Walker-GM Josh-Walker-GM added this to the v6.0.0 milestone Jun 30, 2023
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some inline docs/comments as to how to enable each Otel setting might be helpful.

Also, do these https://redwoodjs.com/docs/graphql#graphql-handler-setup docs need updating for the config options?

@@ -75,6 +75,12 @@ export interface RedwoodGraphQLContext {
[index: string]: unknown
}

export interface RedwoodOpenTelemetryConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps have some inline comments to describe what enabling each does?

for example, setting resolvers to true adds ... ?

This?

          if (options.resolvers) {
             extendContext({
               [tracingSpanSymbol]: executionSpan,

Adds execution span telemetry for each graph resolver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the documentation to the options

@Josh-Walker-GM
Copy link
Collaborator Author

I think we shouldn't update the docs for the docs website until this moves out of experimental. I will update the forum post.

@pvenable
Copy link
Contributor

Thanks, @Josh-Walker-GM, it's great to see this! I look forward to testing it and providing further feedback once it's released.

I can tell you that since our previous correspondence I've also needed to add some custom attributes to our active span (such as the current user ID or other current context), so I would likely be interested in seeing a way to augment the options/attributes passed to startActiveSpan here. This could perhaps be something like a callback that would run in onExecute that could return additional variables to set in the span options, but that's just one idea.

Thanks again for the followup. 🙂

@jtoar jtoar merged commit cf41df3 into main Jun 30, 2023
11 of 12 checks passed
@jtoar jtoar deleted the jgmw-otel/config branch June 30, 2023 20:49
jtoar pushed a commit that referenced this pull request Jun 30, 2023
… updates (#8782)

* Enable options to the OTel plugin

* Start active spans within OTel plugin

* Update setup command

* Conditionally enable resolver wrapping for makeMergedSchema

* Add description to plugin options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: Experimental (?) useRedwoodOpenTelemetry is unconditionally enabled without opt-in as of Redwood 5.x
4 participants