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

Concept of extensions/plugins/instrumentations/events #1374

Open
oprypkhantc opened this issue May 4, 2023 · 4 comments
Open

Concept of extensions/plugins/instrumentations/events #1374

oprypkhantc opened this issue May 4, 2023 · 4 comments

Comments

@oprypkhantc
Copy link
Contributor

oprypkhantc commented May 4, 2023

Hey.

Disclaimer: I've made little research; at this point, this is more of a questionnaire than an issue with a ready solution. My idea is to gather some feedback and see if that's even in the scope of this project before going forward with it.

There are open issues in this project that have no good way of implementing them without tightly coupling graphql-php with the implementation. One example of this is Apollo tracing, but there are more. This is useful for any kind of tracing, logging, custom extensions etc.

One way other GraphQL implementations battle this is by adding "extensions API" (also called "plugins API" and "instrumentations") - those are simple hook points to allow the user to execute arbitrary code on different points of query's execution.

Sometimes the same thing is implemented in form of events - as is the case with Lighthouse. In the scope of graphql-php, we could allow specifying an optional PSR-14 event dispatcher and use it throughout graphql-php.

Would graphql-php be open to address those needs in a similar fashion?

@spawnia
Copy link
Collaborator

spawnia commented May 4, 2023

an optional PSR-14 event dispatcher

I would like to keep this library dependency-free. As of now, it is only coupled to PHP releases and nothing else.

One way other GraphQL implementations battle this is by adding "extensions API" (also called "plugins API" and "instrumentations") - those are simple hook points to allow the user to execute arbitrary code on different points of query's execution.

There are a number of existing extension points in the library already, e.g. validation rules, custom types, pluggable executors. When we allow the library user to add logic of their own, we mostly use a functional style, expecting the library user to provide a callable that is called in certain situtations.

Would graphql-php be open to address those needs in a similar fashion?

I think any solution would have to be tailored to a specific need. If you need to be notified during certain points of the execution, events seem like the way to go. If you need to customize how a certain function is carried out, providing a callable to do it seems right. It just depends.

To summarize, I think we need specific solutions for specific problems. I appreciate you opening this issue, but I don't really see it going anywhere. I don't think we can devise some grand plan for how any and all extensibility of this library should or could work. Feel free to comment on the open issues you allude to or provide a pull request with an idea you have.

@oprypkhantc
Copy link
Contributor Author

oprypkhantc commented May 4, 2023

You might be right about this, but I'm not sure. Let's imagine graphql-php wants to add support for Apollo tracing - obviously, simply hardcoding it is not a good idea because that's not the only tracing implementation out there. A better solution would be for graphql-php to provide some sort of configurable "tracing handler" - a contract with (presumably) executionStarted and executionEnded methods.

That will definitely work, but then the end user wants to also log all GraphQL requests. Would this then require adding a separate "logging handler" with similar executionStarted and executionEnded methods? What happens if that's deemed by graphql-php as out of scope, or it takes too much time, or there's not enough human resources to come up with a good solution for LoggingHandler?

In that scenario users will end up with developers using the wrong tool for the job - i.e. people using tracing handler for logging purposes. This isn't very scalable. I believe that's the reason why PHPUnit has recently added it's event system and deprecated older "plugins" and "hooks" - to provide infinite customizability at a low maintenance cost.

To give some examples of multiple extensions with overlapping "hook points" from Apollo server:

Those are just a tip of the iceberg; there are 3rd party packages and even userland plugins which all "hook" into the same points. It seems like this solution works for multiple purposes at once, if designed well.

Regarding PSR-14, well, it's just a set of contracts. It'd make sense to use them, but if that's a concern, this contract could be defined in graphql-php itself - a single interface with a single dispatch($event) method is all that's really needed. It could then be adapted to PSR-14 or Laravel Dispatcher or whatever in userland.

@ingluisjimenez
Copy link

@spawnia while I understand your concerns, I also think that it would be very helpful to have the ability to add some tracing integrations through events during field resolution, like @oprypkhantc suggested.

I am happy to work on this if you think it has a chance to be reviewed and promoted.

@spawnia
Copy link
Collaborator

spawnia commented Jul 19, 2023

Events seem like a fine way to get notified about what is happening during query execution, sure. As I said, I am open to pull requests.

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

No branches or pull requests

3 participants