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

fix(gatsby): fix broken GraphQL resolver tracing #29015

Merged
merged 2 commits into from Jan 15, 2021

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Jan 13, 2021

Description

Our resolver tracing is broken ATM. Tracing simply counts sync resolver call. It doesn't await the returned promise and ends the activity immediately. So it kinda tracks sync resolvers but not async.

With this change, we track correctly in both cases.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 13, 2021
@vladar vladar added topic: GraphQL Related to Gatsby's GraphQL layer and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 13, 2021
@pvdz
Copy link
Contributor

pvdz commented Jan 14, 2021

Oh yeah we discussed this the other day. The fix is good but the timing will probably be very off since it would encompass anything that happens while the thread is suspended. Anyways, good fix.

return result
}

const endActivity = (): void => {
if (activity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be dropped since we test and shortcut return above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript disagrees.

This file has incomplete typings but if I add proper typings to activity and remove this check I get TS error: TS2339: Property 'end' does not exist on type 'void | IActivity'.   Property 'end' does not exist on type 'void'.

if (activity) {
activity.end()
}
}
if (typeof result?.then === `function`) {
result.then(endActivity, endActivity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this won't swallow the error, I think it will suppress nodejs' builtin reporting for uncaught promises (right? because it sees the error is handled at least somewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a different promise chain than the one returned. So it is not affecting unhandled rejections. Also double-checked with this example in node:

const foo = new Promise(function (resolve, reject) {
  setTimeout(() => {
    reject(new Error("Err"));
  }, 1000);
});

const func = () => {};

foo.then(func, func);

foo.then(() => { console.log(`test`); });

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jan 15, 2021
@gatsbybot gatsbybot merged commit 48db6ac into master Jan 15, 2021
@gatsbybot gatsbybot deleted the vladar/fix-resolver-tracing branch January 15, 2021 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants