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

improving error log for runtime::get_next() curl failure #157

Merged
merged 1 commit into from
Aug 23, 2022
Merged

improving error log for runtime::get_next() curl failure #157

merged 1 commit into from
Aug 23, 2022

Conversation

hbobenicio
Copy link
Contributor

Related to issue #26

Description of changes:
Improves the error log when runtime::get_next() fails (curl error code and error message were only visible as debug logs. This PR promotes them to the error level in order to give users more clue about what is going wrong)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bmoffatt
Copy link
Collaborator

FYI the format check failed https://github.com/awslabs/aws-lambda-cpp/runs/7937034733?check_suite_focus=true, use clang-format to match the project's style

@hbobenicio
Copy link
Contributor Author

hbobenicio commented Aug 22, 2022

FYI the format check failed https://github.com/awslabs/aws-lambda-cpp/runs/7937034733?check_suite_focus=true, use clang-format to match the project's style

My bad. Totally forgot about that. I'm gonna reformat it today. Thanks

@hbobenicio
Copy link
Contributor Author

Done formatting.

Copy link
Contributor

@marcomagdy marcomagdy left a comment

Choose a reason for hiding this comment

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

FWIW my thinking at the time was that curl is an implementation detail that I don't want to expose unless there's a problem I want to debug. Maybe that's too painful and I was wrong.

What do you think of keeping the curl specific error code as "debug" but exposing the endpoint as part of the error message?

@hbobenicio
Copy link
Contributor Author

FWIW my thinking at the time was that curl is an implementation detail that I don't want to expose unless there's a problem I want to debug. Maybe that's too painful and I was wrong.

What do you think of keeping the curl specific error code as "debug" but exposing the endpoint as part of the error message?

I totally see and understand the motivation in hiding implementation details from logs.
I also see no problem in keeping the curl error context at debug level and the endpoint used in the failing request at error level.
In my issue case the ill-formed endpoint would give just enough context for me to realize that something is misconfigured in my environment.

Maybe there is some room for improvement in the startup process like checking the absence of all required environment variables (and listing them out in stderr just before aborting) but that is a whole new issue/suggestion/discussion which doesn't belong exactly here.

So I'm gonna reintroduce the debug log message and let the endpoint in the error one then.
Thanks for the review!

@hbobenicio
Copy link
Contributor Author

  • rebased branch on top of updated master
  • reintroduced the debug log message with the error context from curl
  • added the endpoint info of the failed curl request in the error message

Copy link
Contributor

@marcomagdy marcomagdy left a comment

Choose a reason for hiding this comment

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

Thank you!

@bmoffatt bmoffatt merged commit 9b8daab into awslabs:master Aug 23, 2022
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.

3 participants