-
Notifications
You must be signed in to change notification settings - Fork 219
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
Support GraphQL #850
Support GraphQL #850
Conversation
💚 CLA has been signed |
if "ResolveInfo" == type(query).__name__: | ||
op = query.operation.operation | ||
field = query.field_name | ||
path = ">>".join(query.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variable 'path' is assigned to but never used
❕ Build Aborted
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
Just signed Contributor Agreement |
Hi @RyanKung this is awesome, thank you very much! We do have some planning work done for GraphQL, but this mostly pertains to the server side. You can find that here: https://github.com/elastic/apm/blob/master/docs/graphql.md. Maybe some of it could be relevant for the client side too. As for the span type/subtype/action, I'd personally go with external/graphql/query. There is some prior art in the Node.js agent, where they went with db/graphql/execute, though. To be honest, I'm not super familiar with GraphQL, so I'll have to read up and play around a bit with it before I can give you a proper review. This is probably going to take a few days, sorry about that! |
Turns out there was an issue with uppercasing/lowercasing in our CLA check. It's fixed now! |
Sorry about this, we really need to document how to add testing for a new instrumentation, as it involves quite a few files.
Thanks for response, this PR is also for server side. And I will try to refine this PR according to the document. But do we really needs to show such detail informations like |
from elasticapm.instrumentation.packages.base import AbstractInstrumentedModule | ||
from elasticapm.traces import capture_span | ||
|
||
class GrapheneInstrumentation(AbstractInstrumentedModule): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected 2 blank lines, found 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@RyanKung awesome, your server side transaction naming approach is really neat! In case you missed it, I opened a PR on your fork that adds some more necessary boilerplate to run the tests on our Jenkins CI: RyanKung#1 Agreed that |
add missing test boilerplate
@beniwohli Thanks & Merged, According to the document, the TX name should be changed when getting a GraphQL request. So I have to modify those framework middlewares. Here is what I'm working on now. It may cost few days. Support setup TX name when
Unittest for
|
@RyanKung I wonder if instead of having to add support for all frameworks, we could hook into a deeper part of the GraphQL core library. Having played around with the debugger a bit. The
Another benefit of this would be that we don't depend on the content type to be Something similar would need to be done for the new graphql-core package, maybe here? https://github.com/graphql-python/graphql-core/blob/master/src/graphql/execution/execute.py#L245 |
elasticapm/contrib/flask/__init__.py
Outdated
rule += " GraphQL %s" % get_graphql_tx_name(query, op) | ||
if request.method == "POST": | ||
rule += " GraphQL %s" % get_graphql_tx_name(request.data.decode()) | ||
assert False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not call assert False since python -O removes these calls. Instead callers should raise AssertionError().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@beniwohli Wow, change TX name in hooks is simpler than modify all framework support. I'll try it, Thanks. Maybe I should create a new PR for clear? |
@beniwohli For now this PR is only support I tried to support class GraphQLExecutorInstrumentation(AbstractInstrumentedModule):
name = "graphql"
Instrument_list_2 = [
...
]
instrument_list_3 = [
# For graphql-core 3
("graphql.execution.execute", "ExecutionContext.execute_operation"),
("graphql.execution.execute", "ExecutionContext.execute_fields"),
]
instrument_list = instrument_list_2 + instrument_list_3
def call_graphql_2(self, module, method, wrapped, instance, args, kwargs):
...
def call_graphql_3(self, module, method, wrapped, instance, args, kwargs):
name = "GraphQL"
info = ""
if method == "ExecutionContext.execute_operation":
print(method)
if method == "ExecutionContext.execute_fields":
print(method)
with capture_span(
"%s.%s" % (name, info),
span_type="external",
span_subtype="graphql",
span_action="query"
):
return wrapped(*args, **kwargs)
def call(self, module, method, wrapped, instance, args, kwargs):
if method in dict(self.instrument_list_2).values():
return self.call_graphql_2(module, method, wrapped, instance, args, kwargs)
if method in dict(self.instrument_list_3).values():
return self.call_graphql_3(module, method, wrapped, instance, args, kwargs) Only one of those method will be executed, and the whole result will be @pytest.mark.integrationtest
def test_fetch_data(instrument, elasticapm_client):
query_string = "{succ{yeah},err{__typename}}"
schema = graphene.Schema(query=Query)
elasticapm_client.begin_transaction("transaction.test")
with capture_span("test_graphene", "test"):
result = schema.execute(query_string)
assert result.data == {"succ": {"yeah": "hello world"}, "err": {"__typename": "Error"}} E AssertionError: assert None == {'err': {'__typename': 'Error'}, 'succ': {'yeah': 'hello world'}}
E +None
E -{'succ': {'yeah': 'hello world'}, 'err': {'__typename': 'Error'}} |
Hi, @beniwohli I think graphene3 is in beta stage, so maybe we can only support 2.0 version for now. Graphene3&graph-core3 supporting can be another pull request. |
Hey @RyanKung. Thanks again for all the work you are putting into this, it's shaping up to be a great addition to the agent! I agree that Graphene 3 support can be tackled later on. I did a quick test by adapting two of our test apps, opbeans-python (a Django app, acting as the GraphQL server) and opbeans-flask (a Flask app, acting as a GraphQL client). The waterfall looks a bit noisy to me, however: I wonder if we could either
|
Hi @beniwohli Thanks for reply.
GraphQL query may contains complex/composed queries. A execution may cross a lot of Objects, so I think only top level is not enough for showing complex details.
I added a list of basic type, such as It may avoid capture executions that take very little time. How do you think about it? |
Is there anything I can do to contribute towards this? We are going to move our GraphQL API out of develop and into production in a few weeks, and having native APM support would be invaluable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on this @RyanKung! I'm OK with merging this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Just one nitpick.
@jamesthomasdavidson if you have the time, it would be great if you could give the current master branch a try and see if it fulfills your needs :) |
] | ||
|
||
def get_graphql_tx_name(self, graphql_doc): | ||
op = graphql_doc.definitions[0].operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug here - the first item in the definition is not necessarily an operation.
It can be a fragment in which case you'll get an exception here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in fact, client libraries like Apollo send fragments first and operation last so this will always fail. Its best to iterate the list and find the operation then assume its first\last)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekampf thanks for the heads up! Would you mind opening a new issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekampf Maybe like this? Although I'm not sure how to test that....
def get_graphql_tx_name(self, graphql_doc):
op_def = [
i for i in graphql_doc.definitions
if type(i).__name__=="OperationDefinition"
][0]
op = op_def.operation
fields = op_def.selection_set.selections
return "GraphQL %s %s" % (op.upper(), "+".join([f.name.value for f in fields]))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* support graphene * granphene * unittest * added graphene to test requirement * rm unused variable * support more executor * rm unused import * add missing test boilerplate Sorry about this, we really need to document how to add testing for a new instrumentation, as it involves quite a few files. * support set tx name on flask and django * add empty lines * graphql * rm assert * unittest for django * revert, dont set tx name in middleware * updated graphql support * rename graphene to graphql * rm imports * bugfixed * rm blank line * added missing unittest * rm blank line * setup name * fixed unittest * ignore basic type * fixed test requirement * Update changelog Co-authored-by: Benjamin Wohlwend <beni@elastic.co> Co-authored-by: Colton Myers <colton.myers@gmail.com>
* support graphene * granphene * unittest * added graphene to test requirement * rm unused variable * support more executor * rm unused import * add missing test boilerplate Sorry about this, we really need to document how to add testing for a new instrumentation, as it involves quite a few files. * support set tx name on flask and django * add empty lines * graphql * rm assert * unittest for django * revert, dont set tx name in middleware * updated graphql support * rename graphene to graphql * rm imports * bugfixed * rm blank line * added missing unittest * rm blank line * setup name * fixed unittest * ignore basic type * fixed test requirement * Update changelog Co-authored-by: Benjamin Wohlwend <beni@elastic.co> Co-authored-by: Colton Myers <colton.myers@gmail.com>
* support graphene * granphene * unittest * added graphene to test requirement * rm unused variable * support more executor * rm unused import * add missing test boilerplate Sorry about this, we really need to document how to add testing for a new instrumentation, as it involves quite a few files. * support set tx name on flask and django * add empty lines * graphql * rm assert * unittest for django * revert, dont set tx name in middleware * updated graphql support * rename graphene to graphql * rm imports * bugfixed * rm blank line * added missing unittest * rm blank line * setup name * fixed unittest * ignore basic type * fixed test requirement * Update changelog Co-authored-by: Benjamin Wohlwend <beni@elastic.co> Co-authored-by: Colton Myers <colton.myers@gmail.com>
What does this pull request do?
Support GraphQL
And I'm not sure what
span_type
it should be.Related issues
closes #ISSUE