-
Notifications
You must be signed in to change notification settings - Fork 4
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: log graphql requests #526
Conversation
require 'net/http' | ||
|
||
class SubmitAnalyticsEvent < ApplicationJob | ||
# TODO not entirely sure what goes 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.
^
{ | ||
searchTerms: names, | ||
}, | ||
{ 'dgidb-client-name': 'dgidb-frontend' } |
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.
You might want to double check that it works as expected, but it looks like (based on this example here) you can set an HTTP Header when your initialize the graphql client.
That might be better than stapling it into each query individually.
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.
@acoffman yeah, I was a little unsure of whether we wanted to set different headers for different queries and to filter out noisier or less informative ones. We should probably set a base one at client initialization and then maybe use an additional one for chosen queries if we go that route.
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.
A couple small things, but its looking good overall!
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.
per our discussion, this looks to be capturing what we are intending for our research goals.
approved but I would also defer to @acoffman 's feedback on his earlier requested changes. If those look good, we should be good to go
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.
One minor question, looks good overall!
def analyze_query(query: ) | ||
query_type = query.selected_operation.selections&.first&.name | ||
|
||
return unless query_type == '__schema' |
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.
I think this needs to be inverted right? We skip analytics if it is an introspection query.
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.
Ugh, thanks, I always goof these
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.
LGTM!
* feat: store score components in DB (#518) * fix: update JAX-CKB URL (#521) * feat: jax-ckb -> ckb-core (#522) * fix: chembl version (#524) * feat: define rake task to generate TSVs (#523) * fix: remove redundant field (#527) * build: remove some unused rails boilerplate/imports (#528) * fix: use declared data version value (#525) * fix: remove redundant code (#529) * cicd: update actions revs (#530) * feat: log graphql requests (#526) * feat: log API client (#531) * chore: remove unused readme (#532) * fix: use docm snapshot data (#533) * fix: update api uri (#538) * cicd: add DB check script (#543) * chore: fix a couple of typos/omissions (#541) * fix: use latest GO api changes (#542) * add rows for Download table for 2024 tsvs (#549) * new row for old Downloads file * added one more row --------- Co-authored-by: Cannon <[email protected]> --------- Co-authored-by: James Stevenson <[email protected]> Co-authored-by: Cannon <[email protected]>
* feat: store score components in DB (#518) * fix: update JAX-CKB URL (#521) * feat: jax-ckb -> ckb-core (#522) * fix: chembl version (#524) * feat: define rake task to generate TSVs (#523) * fix: remove redundant field (#527) * build: remove some unused rails boilerplate/imports (#528) * fix: use declared data version value (#525) * fix: remove redundant code (#529) * cicd: update actions revs (#530) * feat: log graphql requests (#526) * feat: log API client (#531) * chore: remove unused readme (#532) * fix: use docm snapshot data (#533) * fix: update api uri (#538) * cicd: add DB check script (#543) * chore: fix a couple of typos/omissions (#541) * fix: use latest GO api changes (#542) * add rows for Download table for 2024 tsvs (#549) * new row for old Downloads file * added one more row --------- Co-authored-by: Cannon <[email protected]> --------- Co-authored-by: James Stevenson <[email protected]> Co-authored-by: Cannon <[email protected]>
This PR does two things
For example, the frontend logs look like:
and the API logs look like
Similarly, I was thinking it'd be nice to grab a reduced list of ambiguous results when those come up, but I haven't had a chance to look through the way to explore the query tree and I'm sort of supposed to get this thing shipped.