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

Improve logging and API errors #538

Closed
nmarcetic opened this issue Jan 9, 2019 · 17 comments · Fixed by #866
Assignees
Labels
Milestone

Comments

@nmarcetic
Copy link
Collaborator

@nmarcetic nmarcetic commented Jan 9, 2019

FEATURE REQUEST

  1. Is there an open issue addressing this request? If it does, please add a "+1" reaction to the
    existing issue, otherwise proceed to step 2.
    No

  2. Describe the feature you are requesting, as well as the possible use case(s) for it.

We need to improve logging and all API response’s, mostly error responses are critical and not useful.

Regarding logging problem,

We use custom logger which is a wrapper around go-kit logger, providing basic methods for different log levels
Mostly logging is handled really well, especially in important places where business logic happens and where logging is crucial. The problem arises on API level where login is happening on transport layer and its very poor, usually errors like malformed entity or similar are not logged at all.
We need to go over all HTTP API logic and check the logging, make it useful.

Regarding API responses problem,

Its similar to logging, API responses are mostly unusable because they returning you just error code with no additional data, no further information, nothing. Error codes are probably the most useful diagnostic element in the API I agree, but there so much cases now where we need to make line between the errors and try to provide a bit more information’s, like a human readable message or even just a context with reference ID.
There is a lot of good resources which can help us to design the response like JSON API specification etc… We should discuss about this, keep it simple, consistent and useful.

Both those problems results with cases where I as API consumer, consume some API end point and get 400 bad request without any explanation and any logs.

@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Jan 11, 2019

Regarding error logging in HTTP package, maybe we can log errors here. What I don't like about this idea, is that some errors are going to be logged twice, once in logging middleware, and second time in HTTP package. We can avoid this by redefining some errors in HTTP package, and log only those.

@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Jan 11, 2019

Regarding error responses that are returned through HTTP, I think that we only need title and description field.

@nmarcetic

This comment has been minimized.

Copy link
Collaborator Author

@nmarcetic nmarcetic commented Jan 28, 2019

@anovakovic01 Totally agree for error responses format. Duplicate logging can be a problem.
I will check out this a bit, would like to hear what others think ?

@dusanb94

This comment has been minimized.

Copy link
Member

@dusanb94 dusanb94 commented Jul 4, 2019

There is also a logger within go-kit server error encoder, so maybe we can utilize it to get logs from the transport layer.

@drasko

This comment has been minimized.

Copy link
Member

@drasko drasko commented Sep 11, 2019

I am taking this one with highest priority.

@drasko drasko modified the milestones: 1.0.0, 0.10.0 Sep 11, 2019
@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Sep 11, 2019

Errors should be handled using this library https://golang.org/pkg/errors/. The current approach to error handling is to use hardcoded errors instead of errors with dynamic messages. I recommend that we wrap errors using fmt.Errorf("... %w ...", ..., err, ...). Also if we want to match two errors we shouldn't use ==. Instead, we should use Is (or As). When a request is received in the transport layer, every service (or method) that this request calls should leave its mark on the error by appending to the error message. Also, if you want to add more value to your errors, create custom error types (there is a good example in this article with time).

@dusanb94

This comment has been minimized.

Copy link
Member

@dusanb94 dusanb94 commented Sep 11, 2019

We have problems with logging only on the transport layer, but those problems will be solved once we update our error handling. We don't have to log anything on the transport layer (which is also good performance-wise): Errors that occurred before request reached our service logic will be included in the response, but we'll keep logs clean in the sense that it's only our service code that we log. It is important to include a proper error message in API response because status code (especially 400) is not particularly useful.

@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Sep 11, 2019

Errors that occurred before request reached our service logic will be included in the response, but we'll keep logs clean in the sense that it's only our service code that we log.

I don't agree with this. We must have all logs logged in one place (Otherwise, you will have to collect logs from both clients and MF services in order to figure out what happened).

@drasko

This comment has been minimized.

Copy link
Member

@drasko drasko commented Sep 12, 2019

@anovakovic01 these were added in Go 1.13: https://golang.org/doc/go1.13

Pretty new stuff.

@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Sep 12, 2019

@drasko we planned to migrate to go 1.13, right? AFAIK, in 1.13 this is part of standard go lib, but it was used even before (it was an independent library).

@nmarcetic

This comment has been minimized.

Copy link
Collaborator Author

@nmarcetic nmarcetic commented Sep 12, 2019

Yes, migrating to Go 1.13 @anovakovic01 @drasko Let's start with modules :)
We should investigate a migration effort.

@drasko

This comment has been minimized.

Copy link
Member

@drasko drasko commented Sep 16, 2019

Errors that occurred before request reached our service logic will be included in the response, but we'll keep logs clean in the sense that it's only our service code that we log.

I don't agree with this. We must have all logs logged in one place (Otherwise, you will have to collect logs from both clients and MF services in order to figure out what happened).

@anovakovic01 currently there is no logging on transport layer - as soon as there is an error, flow is topped and response is retuner to the client: https://github.com/mainflux/mainflux/blob/master/http/api/transport.go#L73

As we return here, we never reach the logging middleware, so no need for wrapping errors.

In my opinion this kind of handling is OK, because this is probably not really Mainflux error - it is client's malformed request (TBH, it can be Mainflux server error as well, as JSON parsing can fail for wahtever reason on the server, and so on).

If we want this logged, we must log it directly on the transport layer IMHO - I di not see what else we can do, having in mind that we return here immediately.

Similarly, we should log service error eiter over there, either wrap them in service domain and unwrap them in transport, just before returning error to the client in this function: https://github.com/mainflux/mainflux/blob/master/http/api/transport.go#L145

@anovakovic01

This comment has been minimized.

Copy link
Member

@anovakovic01 anovakovic01 commented Sep 16, 2019

In my opinion this kind of handling is OK, because this is probably not really Mainflux error - it is client's malformed request (TBH, it can be Mainflux server error as well, as JSON parsing can fail for wahtever reason on the server, and so on).

@drasko And what will you do if it is Mainflux error? We must have logging in transport layer. And when I said that we should log everything in one place, I meant that we should log everything in one service. IMO it doesn't make sense to log part of the errors from MF service on client apps (it will be harder to collect and process everything). Also, what I proposed is that you create middleware for repository interfaces (and other interfaces too). That way we can propagate real error messages (that we get from DB driver for example) to our end logs. Also, if you do this, you'll know what parts of the code were invoked and what where not (this way it will be easier to locate the error origin and the problem).

@drasko drasko assigned blokovi and unassigned drasko Sep 16, 2019
@drasko

This comment has been minimized.

Copy link
Member

@drasko drasko commented Sep 16, 2019

@blokovi please take this one.

There are few things to take care of:

Logging

  • Errors in the transport (like here) should be wrapped
  • They will be unwrapped and logged here - so you need to change error inspecation to use As() and/or Is() from new Go 1.13 errors package
  • Note that you only need to to log transport messages in encodeError(), as Service layer errors wil be logged in logging middleware

Responses

  • Currently we return errors like erroMalformedSubtopic that are not so user-friendly for the client
  • We need to return other, more descriptive error that we construct as a wrapper in transport
  • In encodeError() unwrap and return to client only these descriptive errors (not the actual values that come from Mainflux, as this can be a security breach)
  • Responses should be some form of JSON structure, as mentioned by @nmarcetic in description of this issue.
@knz

This comment has been minimized.

Copy link

@knz knz commented Sep 22, 2019

Note that we've built automatic encode/decode of error objects in http://github.com/cockroachdb/errors due to a use case very similar to yours.

@drasko

This comment has been minimized.

Copy link
Member

@drasko drasko commented Sep 23, 2019

@knz interesting. Looks like this package has some adventages over the Go 1.13 stdlib package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.