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

Add query execution context based tracing for API requests #4077

Conversation

toneymathews
Copy link
Contributor

@toneymathews toneymathews commented May 26, 2022

Closes #4001

An attempt at allowing tracing per request as per suggestions in #4001

  • cached_platform_key now accepts the trace phase as a third parameter.
  • OpenTelemetryTracing overrides cached_platform_key to access execution context not available in platform_field_key, platform_authorized_key, platform_resolve_type_key

This PR includes

  • modified copy of the OpenTelemetry implementation to demonstrate how we would make use of PlatformTracing
  • tests to ensure the above works as expected
  • a stub for the OpenTelemetry agent

CI has failed due to rubocop failures in files not associated with this PR.

If this looks good, we are hoping to add this to 1.13.x (< 2) since we haven't yet migrated.

@rmosolgo
Copy link
Owner

Awesome, thanks for sharing this improvement! Yep, I'll backport it to 1.13.x and release a new version there shortly.

@rmosolgo rmosolgo merged commit fea435f into rmosolgo:master May 31, 2022
@rmosolgo
Copy link
Owner

🚢 in 1.13.13 !

@toneymathews
Copy link
Contributor Author

Thanks a lot, much appreciated :)

@ravangen
Copy link
Contributor

ravangen commented May 31, 2022

🤔 Do you want to maintain OpenTelemetryTracing going forward, or should we leave that to open-telemetry/opentelemetry-ruby?

It was originally included here to demonstrate how everything works together across gems.

@rmosolgo
Copy link
Owner

Either way works for me 🤷

@ravangen
Copy link
Contributor

ravangen commented May 31, 2022

I realize you just shipped a release, but I was unsure if we verified/documented how it integrates with opentelemetry in a production environment (e.g. loading its config and tracer). The tests verify behaviour but not integration. I was hoping to follow up this PR with one for opentelemetry-ruby now that cached_platform_key signature was updated.

I'm not sure if/how copyright plays a role in this decision either, we basically forked the original.

IMO we remove OpenTelemetryTracing from graphql-ruby and perhaps the docs link to its existence? Sorry about all this.

@fbogsany
Copy link

First-party OpenTelemetry instrumentation is a wonderful thing. We very much want to encourage this. It really should not depend on the OpenTelemetry Ruby auto-instrumentation in any way, though. The goal should be to deprecate the OpenTelemetry auto-instrumentation in favour of first-party instrumentation. There should be no reference to OpenTelemetry::Instrumentation::GraphQL in this repo.

I'll look at the licensing issues, but speaking for myself, I have no problem with copying any of the instrumentation code from the OpenTelemetry Ruby GraphQL auto-instrumentation into this repo, with or without attribution.

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

Successfully merging this pull request may close these issues.

PlatformTracing per request trace configuration
4 participants