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

PlatformTracing per request trace configuration #4001

Closed
ravangen opened this issue Mar 22, 2022 · 4 comments · Fixed by #4077 · May be fixed by toneymathews/graphql-ruby#4
Closed

PlatformTracing per request trace configuration #4001

ravangen opened this issue Mar 22, 2022 · 4 comments · Fixed by #4077 · May be fixed by toneymathews/graphql-ruby#4

Comments

@ravangen
Copy link
Contributor

Is your feature request related to a problem? Please describe.

With opentelemetry-instrumentation-graphql, a GraphQLTracer is provided based on GraphQL::Tracing::PlatformTracing.

It provides several schema level configuration options that can enable/disable specific parts of tracing.

I am looking for a way to manage these options per execution instead of being constant. For example, a HTTP header may request verbose traces be recorded, ideally enabling enable_platform_field for only this request's GraphQL execution.

Describe the solution you'd like

When building the platform_key (here, here, here), I wonder if it makes sense to optionally provide context. This would enable platform_field_key/platform_resolve_type_key/platform_authorized_key to make request specific determinations based on the GraphQL execution context.

For example, something like this would now be possible:

def platform_field_key(type, field, context)
  return unless config[:enable_platform_field] || context.namespace(:opentelemetry)[:enable_platform_field]

  "#{type.graphql_name}.#{field.graphql_name}"
end

The platform key cache should not be impacted as it keeps it data under context, regenerated for each request.

Additional context

I imagine this would be a nice to have for anything using PlatformTracing, not only Open Telemetry.

@rmosolgo
Copy link
Owner

👍 Yeah, makes sense to me to accept context one way or another. If it was me, I'd add a new parameter to cached_platform_key, telling what kind of key to make, for example, cached_platform_key(ctx, :authorized, type) { ... }. Then, you could override cached_platform_key in your own tracer to read from context as appropriate. It'd be nice to keep the existing method signatures as-is, for compatibility, if possible.

@toneymathews
Copy link
Contributor

We tried to implement your suggestion, but we found that the GraphQL type information (data[:owner]) was not available to us in this approach.

To show our initial suggestion, I've created this demonstration as well. Not sure how best to avoid breaking the existing method signature for PlatformTracing implementations outside of the gem.

Let us know your thoughts how we can proceed on this. Thanks!

@rmosolgo
Copy link
Owner

rmosolgo commented Apr 12, 2022

Ahh... it receives execution context, not the trace data. Good news is, in recent GraphQL-Ruby versions (probably 1.11+?), context contains some runtime metadata that would probably work:

  • current_object: the GraphQL type object
  • current_field: the field being resolved
  • current_arguments: the arguments to that field
  • current_path: the result path (eg, including integer list indexes) for the result of the current field

They're set during execution by this method:

def set_all_interpreter_context(object, field, arguments, path)
if object
@context[:current_object] = @interpreter_context[:current_object] = object
end
if field
@context[:current_field] = @interpreter_context[:current_field] = field
end
if arguments
@context[:current_arguments] = @interpreter_context[:current_arguments] = arguments
end
if path
@context[:current_path] = @interpreter_context[:current_path] = path
end
end

I bet context[:current_object] would have data[:owner], or if I'm mistaken, maybe context[:current_field].owner would work.

How about giving that a shot?

@toneymathews
Copy link
Contributor

We've added #4077 implementing the above suggestions. Can you pls take a look if it looks good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants