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

feat: show graphql operation name for graphql requests #1521

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yurik256
Copy link

@yurik256 yurik256 commented Nov 8, 2024

Please verify the following:

  • yarn build-and-test:local passes
  • I have added tests for any new features, if relevant
  • README.md (or relevant documentation) has been updated with your changes

Describe your PR

On the react native project I'm working on, we are extensively using graphql.
Current implement of timeline plugin, makes it hard to differentiate graphql requests, as they are all shown as POST /graphql in the timeline

This PR improves this by adding the following

  • show operation name for graphql requests
    • This is implemented by parsing request.operationName field, which is automatically added for all graphql requests made by @apollo/client
  • Ability to filter requests by request data

Screenshots

Show operation name
Screenshot 2024-11-08 at 10 41 40 AM

Ability to search by request data ( in this case, operation name )
Screenshot 2024-11-08 at 10 41 59 AM

@yurik256
Copy link
Author

The contributing guide, unfortunately, doesn't specify who should be marked as the reviewer.
@morganick, I see that you reviewed a few recent PRs, would you mind taking a look?

Copy link
Contributor

@morganick morganick left a comment

Choose a reason for hiding this comment

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

Sorry for such a long delay in this. I had a couple of moments this morning and had the tab still open 😅 I haven't had a chance to fully test this out with a GraphQL API & Apollo Client. I'll look at doing that next week.

@frankcalise
Copy link
Contributor

@yurik256 thanks for the contribution! Do you have the time to make the requested changes by Nick? If not let us know and we can try to bring it up to speed.

@yurik256
Copy link
Author

@yurik256 thanks for the contribution! Do you have the time to make the requested changes by Nick? If not let us know and we can try to bring it up to speed.

@frankcalise thanks for following up! I somehow missed the message from @morganick :(
The suggested change to use join makes sense. Just pushed the changes.

I tried changing the order so that we show the operation name before the URL, and that makes it visually hard to parse.
It could be that I'm also just used to the fact that URL should come first 🤷‍♂️
I would like to propose that we display the operation name after the URL.
@morganick, what do you think?

operation name before:
image

operation name after:
384430781-bc74fd3c-c776-417a-a588-55ed812e9e05

@morganick
Copy link
Contributor

@yurik256 I'm good with putting it after the URL. I'd love to update the design of the timeline to accommodate for things like this. It gets lost a bit either way, but I agree that after is easier on the eyes because of the uniformity of the pattern in POST /url/goes/here

@yurik256 yurik256 requested a review from morganick December 20, 2024 15:17
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.

3 participants