-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Attempt to illustrate execute phase bug #868
Conversation
🦋 Changeset detectedLatest commit: e965f69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
3349735
to
bdfca60
Compare
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.
Looks very nice. Super cool you added a test. To me it looks good. Left some bits. Will have another look tomorrow with fresh eyes.
const schema = require('@graphql-tools/schema'); | ||
|
||
const { makeExecutableSchema } = schema; | ||
const { getDirective, mapSchema, MapperKind } = utils; |
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.
Nit: you can destructure this in the return of the require.
schema, | ||
fieldConfig, | ||
directiveName | ||
)?.[0]; |
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.
For readability I would not derefernce here. We can just add another firstDirective variable maybe which we either assign or destructure to.
type Book { | ||
title: String | ||
author: String | ||
isbn: String @error |
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.
Nice.
const parsedMetrics = parsePrometheusTextFormat(rawMetrics); | ||
const graphQlErrorsTotal = parsedMetrics.find( | ||
(metric) => metric.name === 'graphql_errors_total' | ||
).metrics; |
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.
💯
bdfca60
to
a26a33a
Compare
ensure that GraphQL errors thrown in execute phase are counted and that the plugin does not itself throw when it encounters such an error
a26a33a
to
f4d06c7
Compare
Codecov Report
@@ Coverage Diff @@
## main #868 +/- ##
==========================================
+ Coverage 86.75% 87.15% +0.39%
==========================================
Files 22 22
Lines 506 506
Branches 119 119
==========================================
+ Hits 439 441 +2
+ Misses 58 56 -2
Partials 9 9
|
Summary
This is to demonstrate a potential bug with capturing error metrics in execute phase, and possible fix.
Description
Errors are not recorded when thrown in execute phase.
Technical debt & future