Skip to content

fix: log CloudEvent function errors to stderr #132

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

Merged
merged 3 commits into from
Jun 9, 2022

Conversation

anniefu
Copy link
Contributor

@anniefu anniefu commented Jun 8, 2022

CloudEvent functions currently return 500 when an error is returned, but
do not do anything with the error message. This logs the error message
to stderr so it is not lost.

Unlike Background Functions, the error message is NOT included in the
response body though.

Also, add fields for checking stderr to the unit tests.

CloudEvent functions currently return 500 when an error is returned, but
do not do anything with the error message. This logs the error message
to stderr so it is not lost.

Unlike Background Functions, the error message is NOT included in the
response body though.

Also, add fields for checking stderr to the unit tests.
@anniefu anniefu requested review from matthewrobertson and grant June 8, 2022 22:51
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

Requesting changes with one comment.

@anniefu anniefu requested a review from grant June 9, 2022 18:09
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

1 comment

@anniefu anniefu requested a review from grant June 9, 2022 18:35
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

I find it odd that we're checking the stderr string "Function error: TestEventFunction(erroring function): this error should fire" for just this one tc / possible function error, but lgtm other than that.

@anniefu
Copy link
Contributor Author

anniefu commented Jun 9, 2022

I find it odd that we're checking the stderr string "Function error: TestEventFunction(erroring function): this error should fire" for just this one tc / possible function error, but lgtm other than that.

Most of the other test cases do not return an error, so there is no stderr to check for. Note, that particular test case has a function that returns an error with the error message being "TestEventFunction(erroring function): this error should fire".

@anniefu anniefu merged commit ac973b4 into GoogleCloudPlatform:master Jun 9, 2022
@grant
Copy link
Contributor

grant commented Jun 9, 2022

Note, that particular test case has a function that returns an error with the error message being "TestEventFunction(erroring function): this error should fire".

The line fmt.Sprintf(fnErrorMessageStderrTmpl, "TestEventFunction(erroring function): this error should fire")

Resolves to the HTTP body response:

"Function error: TestEventFunction(erroring function): this error should fire")

Right?

@anniefu
Copy link
Contributor Author

anniefu commented Jun 9, 2022

Note, that particular test case has a function that returns an error with the error message being "TestEventFunction(erroring function): this error should fire".

The line fmt.Sprintf(fnErrorMessageStderrTmpl, "TestEventFunction(erroring function): this error should fire")

Resolves to the HTTP body response:

"Function error: TestEventFunction(erroring function): this error should fire")

Right?

Yes. That test case has always returned that string in the response body, it just wasn't being checked due to an if-condition that skipped checking the response body if wantRespBody is empty: https://github.com/GoogleCloudPlatform/functions-framework-go/pull/132/files#diff-ca9e477292e26295fedc9f881c2e359cd12ab7ea23de39d5373fe7b868d48c50R331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants