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

chore: add log for easy debug #1253

Merged
merged 1 commit into from Jul 11, 2019
Merged

chore: add log for easy debug #1253

merged 1 commit into from Jul 11, 2019

Conversation

haozibi
Copy link
Contributor

@haozibi haozibi commented Jul 10, 2019

Description

Just add logs to watchdog for easy debugging

Motivation and Context

  • I have raised an issue to propose this change (required)
  • My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

No tests are required

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@derek
Copy link

derek bot commented Jul 10, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot added the no-dco label Jul 10, 2019
Signed-off-by: haozibi <haozibi@foxmail.com>
@derek derek bot removed the no-dco label Jul 10, 2019
@alexellis
Copy link
Member

You should be able to get this logging by toggling one of the environment variables.

Which ones have you tried? Did they work for you? https://github.com/openfaas/faas/tree/master/watchdog

@haozibi
Copy link
Contributor Author

haozibi commented Jul 10, 2019

@alexellis hello

I have seen the environment variables of watchdog. When I debug the program, I enabled the write_debug environment variable in the program, but the watchdog will not output any error if it reads the error when reading the gateway request, let me think there is no problem, so I submitted this simple pr.

And I saw that there is no environment variable to open this error output.

@alexellis
Copy link
Member

I am OK with the idea, but I'd like to see it as write_debug_error perhaps? Also is the error format consistent with the other messages?

@haozibi
Copy link
Contributor Author

haozibi commented Jul 10, 2019

The error message is written in the format

faas/watchdog/handler.go

Lines 164 to 168 in b572e77

if err != nil {
if config.writeDebug == true {
log.Printf("Success=%t, Error=%s\n", targetCmd.ProcessState.Success(), err.Error())
log.Printf("Out=%s\n", out)
}

I think adding an environment variable for simple log output is a bit complicated.

Do you have any good suggestions?

@alexellis
Copy link
Member

I'm weary of adding additional log messages.

I guess my question would be, why did "buildFunctionInput" throw an error for you? How were you using it?

@haozibi
Copy link
Contributor Author

haozibi commented Jul 11, 2019

I used a LoadBalance before the gateway. Due to the LoadBalance bug, the gateway sends the request to the watchdog very slowly. If the request body is very large, it may cause io.readAll to not read completely and then report the unexpected EOF. You can see this error (error with upstream request to: , Post http://xxx.openfaas-fn.svc.cluster.local:8080: unexpected EOF), but I can't see it explicitly in the watchdog.

I just think that adding one will be more obvious, and the error returned by the gateway is a bit too simple.

+----------------+        +--------------------+        +-----------------+
|                |        |                    |        |                 |
|  LoadBalance   +------->+    Gateway         +------->+    watchDog     |
|                |        |                    |        |                 |
+----------------+        +--------------------+        +-----------------+

@alexellis
Copy link
Member

Sounds good to me. Thank you for explaining it.

@alexellis alexellis merged commit e8b3818 into openfaas:master Jul 11, 2019
@alexellis
Copy link
Member

Thanks for your first PR. You're now a contributor and may appear on openfaas.com.

You can join Slack and chat with us more.

It would be great to understand more of what you're building and how you are using the project. We also have other areas where contributions are needed.

--
Join Slack to connect with the community
https://docs.openfaas.com/community

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

Successfully merging this pull request may close these issues.

None yet

2 participants