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

Hooks for graphql parse and validate functions #1167

Closed
marija17 opened this issue Dec 1, 2020 · 11 comments
Closed

Hooks for graphql parse and validate functions #1167

marija17 opened this issue Dec 1, 2020 · 11 comments

Comments

@marija17
Copy link

marija17 commented Dec 1, 2020

It would be great if we were able to add hooks to the GraphQL parse and validate functions, similar to what exists already for execute: https://github.com/graphql/graphql-js

Is there interest and/or a plan in implementing this in the future? Are there particular challenges or concerns about adding it?

I'd be interested in contributing to the repo myself if this is something likely to be considered for release.

@rochdev
Copy link
Member

rochdev commented Dec 2, 2020

Is there interest and/or a plan in implementing this in the future?

The original use case was only for execute but ideally there would be hooks for everything.

Are there particular challenges or concerns about adding it?

The main challenge for every hook is always to figure out what makes the most sense to pass to the hook. Implementing the hook itself is usually not a lot of code.

I'd be interested in contributing to the repo myself if this is something likely to be considered for release.

That would definitely be considered and contributions are welcome!

@quinnlangille
Copy link

quinnlangille commented Dec 28, 2020

@rochdev I took at a look at implementing hooks for parse and validate today, and it seems like they each have access to totally different data from execute on the graphql-js end. It also seems like neither has access to variable values. I'm pretty new to dd-trace plugins, but if I mostly cared about adding variable values to my parse and validate spans, what would be the best way to go about that?

I have access to the values during the parse and validate step in my application, is there anything I can do from that side (outside of adding a new span) that would let me pass them into the parse and validate hooks? I'm available to make the contribution for the new hooks, but if you could give some direction on how to pass the variable info to them that would be great!

@rochdev
Copy link
Member

rochdev commented Jan 5, 2021

@quinnlangille The main issue is that variable values only make sense in the context of the execution since they are applied to the result of the parse step in the execute step. What is your use case? Maybe it would be possible to combine a parse hook and an execute hook to achieve what you're looking for.

@quinnlangille
Copy link

I mostly just want to see the variables in the same span (trace would also be fine) as the query they're used in. We have both the query and variables printing in the execute step now, but on requests that fail on parse, validate or generally before the execute step we can only see the query. Being able to pass the variables to a parse/validate hook (like we do for execute) would help with debugging since we could easily recreate the failing request. We have access to the variables during parse/validate in the app but I'm not sure if/how I can pass them into the appropriate span.

Maybe it would be possible to combine a parse hook and an execute hook to achieve what you're looking for.

Not 100% what you mean here, is there a way to do this with dd-trace? Sorry if I'm missing something obvious lol again, pretty new to dd-trace

@thehideki81
Copy link
Contributor

I have a local fork for adding hooks for validate and parse, with tests implemented. Would it be possible to make a PR for review? @rochdev ?

@thehideki81
Copy link
Contributor

thehideki81 commented Jul 13, 2021

my implementation will not add validation as arguments, decided to go with the following, since in my opinion these are the most useful for these use cases: to modify and add spans.

(span, document, errors) for validation
(span, document, operation) for parse

@thehideki81
Copy link
Contributor

thehideki81 commented Jul 13, 2021

I have created a PR about adding parse and validate hooks.

#1495

Would appreciate help on getting this forward.

EDIT: Fixed tests for PR.

@rochdev
Copy link
Member

rochdev commented Jul 16, 2021

I have created a PR about adding parse and validate hooks.

Thanks! I took a quick look earlier today and it's fine for the most part, just want to make sure of the arguments to the hooks. Let's continue the discussion in the PR.

@rochdev
Copy link
Member

rochdev commented Jul 16, 2021

We have access to the variables during parse/validate in the app but I'm not sure if/how I can pass them into the appropriate span.

@quinnlangille Sorry for the late reply, I completely missed your comment. Is this still an issue for you? Can you share the code where the variables are available? Variables are only relevant from the perspective of the execution since the parsing and validation only work on the schema and the document, not on variables. Because of that, they are not directly available to parse internally. You could make them available by adding them to the request span as a baggage item, and retrieving them later from the parse hook from #1495 when it lands to add them to the spans. However, my understanding is that the variables are only relevant in the context of the execution, and parsing and validation are unrelated to variables, so I think I need to better understand your use case to make sure there isn't another easier way to address it.

@quinnlangille
Copy link

Hey @rochdev, we managed to get the data we needed with just some regular logging outside of the trace. Going as granular as the trace our first instinct, but ended up being more of a nice to have. If the hooks were made available though (re: above comments from @thehideki81) then we could definitely make use of them

@rochdev rochdev added this to the 1.1.0 milestone Jul 21, 2021
@rochdev
Copy link
Member

rochdev commented Jul 21, 2021

The parse/validate hooks were just released in 1.1.0. Thanks @thehideki81 for implementing this!

@rochdev rochdev closed this as completed Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants