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

Laravel Lighthouse tracing integration #490

Merged
merged 10 commits into from
May 25, 2021
Merged

Conversation

stayallive
Copy link
Collaborator

This PR aims to add out of the box tracing support for Laravel Lighthouse.

Sneak peek of how a batched request executing multiple GraphQL queries would look like:

image

/cc @spawnia

@spawnia
Copy link
Contributor

spawnia commented May 18, 2021

I think it would be better to use only the top-level fields for naming and grouping operations. That makes it very predictable, whereas using operation names and aliases is dependent on the client and might change on a whim.

If tracing on the level of named operations is wanted, this can still happen from the client side. In integrated scenarios, the clients are most likely using Sentry too 😉

@stayallive
Copy link
Collaborator Author

Yeah, I do not agree on that. The operation name is super useful IMHO, but I also see where you are coming from and I might even consider adding a configuration option or something else so you can switch between the two without too much work on the integrator.

@spawnia
Copy link
Contributor

spawnia commented May 19, 2021

I can see how both approaches might be desirable in different scenarios. We have multiple clients using our API, so using operation names does not work well for us. It might group operations where the fields are completely distinct but coincidentally named the same, and fail to group operations where the fields are actually the same but named differently.

@stayallive
Copy link
Collaborator Author

Yeah exactly, I think having both is the best option here because it would depend on the use cases. For us the difference between query{viewer} and query{viewer} would be desirable and could be indicated by the operation or query name because although they only query the viewer they can have a vastly different subselection of fields.

@spawnia
Copy link
Contributor

spawnia commented May 19, 2021

I am quite excited to try this out!

@stayallive
Copy link
Collaborator Author

stayallive commented May 19, 2021

So what about this for changing if the integration ignores operation names?

<?php

namespace App\Providers;

use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    public function register()
    {
+       $this->app->when(
+           \Sentry\Laravel\Tracing\Integrations\LighthouseIntegration::class
+       )->needs('$ignoreOperationName')->give(true);
    }
}

This seems like a pretty cool solution to prevent needing to make life too difficult?

@spawnia
Copy link
Contributor

spawnia commented May 19, 2021

@stayallive Woah, that usage of the Container is pretty advanced. Looks acceptable to me.

@stayallive
Copy link
Collaborator Author

stayallive commented May 19, 2021

I have started testing this branch on our staging environment and I'm pretty excited for this already!

image

You can test it by making the following change in your composer.json (and reverting that later!):

-   "sentry/sentry-laravel": "^2.5",
+   "sentry/sentry-laravel": "dev-lighthouse-integration#78f560d892aee0a92295e63af4d4fc8d7a4f3519",

No other configuration should be needed "out of the box" apart from setting up tracing in the first place.

If you want to ignore operation names passed by the client and want to group transactions on the root selection set instead you can add the following snippet to the register method of for example your AppServiceProvider:

$this->app->when(
    \Sentry\Laravel\Tracing\Integrations\LighthouseIntegration::class
)->needs('$ignoreOperationName')->give(true);

@stayallive stayallive marked this pull request as ready for review May 19, 2021 22:13
@spawnia
Copy link
Contributor

spawnia commented May 25, 2021

Before and after in this screenshot, it is a great improvement:

image

Copy link
Contributor

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Eagerly awaiting the release 🚢

@stayallive stayallive merged commit 6a64f65 into master May 25, 2021
@stayallive stayallive deleted the lighthouse-integration branch May 25, 2021 12:52
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.

None yet

3 participants