-
Notifications
You must be signed in to change notification settings - Fork 160
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
Unable to grab references to uncaught exceptions #215
Comments
Hi @ajjindal, I'm not able to reproduce the issue. Can you provide a clear, minimal setup for reproducing this issue? For example, couldn't you just wrap your function in a try/catch? Are you saying the framework itself is throwing an exception that you want to catch? For creating a global exception handler, I believe we could use Express middleware to register a global exception handler to all routes. I have a blogpost with using middleware: Let me know how to help, or what you're looking for specifically. |
To clarify, this is not a bug but more like an ask to support handling of uncaught exceptions through third party like Sentry. I am looking for a pointer to the handler to be able to grab uncaught exceptions and report them into Sentry.
Gave more details to Steren and Vinod. |
OK, thanks for some more details. Would it be okay to catch uncaught exceptions with the Node process and handle them with Sentry? process.on('uncaughtException', function (err) {
Sentry.captureException(err);
Sentry.flush(2000);
}); Or listen to before the process exits: process.on('beforeExit', async () => {
await something();
process.exit(0); // if you don't close yourself this will run forever
}); https://nodejs.org/docs/latest-v12.x/api/process.html I believe these event handlers can be sync or async functions. |
Sorry for late response. I was checking on the recommendation with our tech lead. Yes, that + |
I'm not sure what you mean by this. With the above suggestion, you can catch all Node exceptions in a function handler. What do you mean by If the exception is not coming from the Functions Framework itself, then can it be caught with the function handler? |
To be clear: We expect you to be able to use |
oh that makes sense. we use the combination in our Node SDK but your suggestion makes sense. Will love to get |
|
I must be missing something. Let me discuss internally and get back to the thread here. |
So, the Thank you for prompt responses. |
@ajjindal Is there anything else regarding grabbing uncaught exceptions that we can do? #215 (comment) provides some code samples and links to docs. If not, can I close this issue? |
I am good for now. Please close the issue. I can come back to this if we run into issues. |
@grant We are trying to integrate now and looks like the problem with uncaught exceptions still exists. We are doing functions-framework-nodejs/src/invoker.ts Lines 255 to 258 in 63cb628
Is it possible to support graceful termination? Can we open the issue again? |
I believe multiple event handlers are possible. It might require the function to be deployed on Node.js 12+. Did you try |
@marshall-lee can you help me with a response here? |
Yep, that's possible. In fact, Sentry's node integration already sets this It ends up invoking an asynchronous operation called So basically, having this
According to node'js docs:
When uncaught exception occurs, there could be some work in the event loop. I think this event is more suitable for normal termination. As for
So it's not suitable for calling |
So what we probably need here is the ability to set some custom framework handler for uncaught exceptions. Framework would |
I'm going to loop back with the Functions team about this. Would a delay of 5 seconds or so before sending the HTTP response on https://github.com/GoogleCloudPlatform/functions-framework-nodejs/blob/master/src/invoker.ts#L255 @marshall-lee Can you perhaps fork this repo and try to alter the framework such that the integration works on your end? If you need instructions for deploying a forked version on to Cloud Functions, let me know. You can deploy the forked version on Cloud Run more easily since you have full control of the container. See this blogpost for an example. |
Re: #215 (comment) @ajjindal @marshall-lee Can you let us know what change you're looking for, or if this solution would work? |
@marshall-lee is planning on creating a PR in the next couple days. |
@grant let us know if this approach will work. |
Pardon the delay. Having 1+ user defined async global error handlers would be fine, but I don't think there's a concrete proposed design. Can you describe a bit more how you're going to start the framework? Questions:
Proposed interfaceconst framework = require('@google-cloud/functions-framework');
const origCallback = framework.ERROR_HANDLER.onUncaughtExceptionCallback;
framework.ERROR_HANDLER.onUncaughtExceptionCallback = (err) => {
flush(timeout).then(() => origCallback(err));
}; Suggested interfaceconst framework = require('@google-cloud/functions-framework');
// Listen to https://nodejs.org/api/process.html#process_event_uncaughtexception
framework.on('uncaughtException', async (err) => {
await flush(timeout);
};
I was creating a prototype, but the design most common way of using this library as a CLI isn't described. https://github.com/GoogleCloudPlatform/functions-framework-nodejs/compare/grant_custom_errors It would also help me if you had an example piece of code that is expected to throw some exception so I can try things out. Here are some I created: const process = require('process');
// Test uncaughtException
process.on('uncaughtException', () => {
console.log('test uncaughtException');
});
// Test exit
process.on('exit', () => {
console.log('test exit');
});
throw new Error('oh, hello there!');
// TEST SIGINT
process.on('SIGINT', () => {
console.log('test exit');
});
let i = 0;
while (i < 100000000) {
++i;
} |
For now I only tested my drafted approach starting
and
With these global exception handlers I'm trying to catch any exception thrown outside of a function. Such exceptions are rare but they must be caught and sent to Sentry too. As for exceptions thrown during execution of a function, we catch them using When wrapping functions, we both use look |
This is the // index.js
const Sentry = require("@sentry/serverless");
const framework = require("@google-cloud/functions-framework");
Sentry.GCPFunction.init({
dsn: "http://123123123123123123123123123@localhost:9000/5",
tracesSampleRate: 1.0,
});
// This piece of code must be somewhere in `Sentry.GCPFunction.init`:
const orig = framework.ERROR_HANDLER.onUncaughtExceptionCallback;
framework.ERROR_HANDLER.onUncaughtExceptionCallback = function(err) {
console.log(`CAUGHT ${err}`);
Sentry.captureException(err);
Sentry.flush().then(() => {
orig(err);
});
}
setTimeout(() => {
throw new Error('bad luck');
}, 10);
exports.helloWorld = Sentry.GCPFunction.wrapHttpFunction((req, res) => {
res.send('ok');
}); |
@grant I like your suggested interface more! Mine is just a PoC illustration. |
Thanks for the sample. This PR #229 (comment) uses the Functions Framework as a module with From the above sample, it looks like you're wishing to require the module as well.
OK.
Thanks. I think we should implement this interface: const framework = require('@google-cloud/functions-framework');
// Listen to https://nodejs.org/api/process.html#process_event_uncaughtexception
framework.on('uncaughtException', async (err) => {
await flush(timeout);
}; For that to work, with the use case of I think we have to delay registering existing global error handler when this library is used as a module (i.e. required). Functions Framework Interfacefunctions-framework-nodejs/src/index.ts Lines 109 to 117 in 01b7df3
if (require.main !== module) {
console.log('required as a module');
// listen to the server as normal
}
export function start() {
// actually do listening...
} ...and user code: const framework = require("@google-cloud/functions-framework");
framework.on('uncaughtException', (error) => { ... });
framework.start();
functions-framework-nodejs/src/invoker.ts Lines 254 to 281 in 01b7df3
|
The thing with starting the framework with... const framework = require("@google-cloud/functions-framework"); ...is that currently the Functions Framework registers error handlers immediately, so you cannot add custom handlers. That's why we'd need to register handlers and start separately: const framework = require("@google-cloud/functions-framework");
framework.on('uncaughtException', (error) => { ... });
framework.start(); |
I work at Sentry, an open source software for application monitoring. We are adding support for Node+Cloud Functions to automatically report code errors. However, we are not able to get reference to the handler in order to capture uncaught exceptions. developers are required to manually catch the error and report it (https://github.com/GoogleCloudPlatform/stackdriver-errors-js#usage).
Here's current Sentry Node integration on Cloud Functions (https://docs.sentry.io/platforms/node/guides/gcp-functions/). This requires captureException call.
As a result of conversation with Steren and Vinod, posting the issue here.
cc: @steren @grant
The text was updated successfully, but these errors were encountered: