Add common logger with specific fields

[addudko]

No description provided.

[drewwells]

this entire block is implementation specific and does not belong in the opensource project. I think you want to implement ways to instruct the toolkit to parse out specific fields, namely ophid. I believe this field will become less useful in the future as account ID is included ( in all cases ) for a JWT

[burov]

Agree with Drew, we should avoid exposing implementation specific details into open source projects

[burov]

Instead of trying to solve specific problems we should focus on providing general and extensible tool which helps people to get what they needs

[addudko]

Had the same thoughts regarding the ophid, but according to docs I understood this like we need logger with specific fields, set by default and not customizable.

[burov]

It's better to rewrite this block in a more Go way and awoid if ... else cases

log.SetLevel(logrus.InfoLevel)
if lvl, ok = logLevels[level]; ok {
    log.SetLevel(lvl)
    return log
}

log.Errorf("Invalid %q provided for log level", lvl)
return log

[burov]

Please add blank line before return statement, it will male code more readable

[addudko]

I'm not totally agree with this particular case, because it's perfectly readable I think. But I've changed it

[burov]

Please add blank line before return statement, it will male code more readable

[burov]

Agree with Drew, we should avoid exposing implementation specific details into open source projects

[burov]

Instead of trying to solve specific problems we should focus on providing general and extensible tool which helps people to get what they needs

[burov]

This block has poor readability , it will be better to reduce LOC between initialization and actual usage of variable, as well please separe each field by blank line

startTime := time.Now()

fields := newLoggerFields(method, startTime, clientKind)
addSpecificFields(ctx, fields, logger, o)

err := invoker(ctx, method, req, reply, cc, opts...)
if err != nil {
 	fields[logrus.ErrorKey] = err
}

code := o.codeFunc(err)
fields[GRPCCodeKey] = code.String()

durField, durVal := o.durationFunc(time.Since(startTime))
fields[durField] = durVal

level := o.levelFunc(code)
levelLogf(logrus.NewEntry(logger).WithFields(fields), level, "finished unary call with code "+code.String())

return err

[burov]

From my point of view we can initialize defaultOptions per request instad of using global variable for this, this helps us to avoid such magic, simple is better than clever

[burov]

I don't see this options can be changed by client, in this case i don't think this is a good idea to add them into options pattern

[burov]

As i mentioned before, this is special case for a few our applications, suggest to avoid introducing this into common logger

[burov]

Should we instead of using simple string define custom type

type logLevel logrus.Level

and define a few public constansts

const (
   DebugLevel = logrus.DebugLevel
   InfoLevel = logrus.InfoLevel
   ...
)

This helps us to make this function safe and forbide clients pass bad level

[addudko]

I think it's better to leave this argument as is and use already written levels from logrus. Don't see reasons to rewrite them

[marcinas]

with the above suggested changes, this looks mostly good
this may not be the PR for this, but before the common logger is to be widely distributed, we should incorporate the new logging requirements, as detailed in https://docs.google.com/document/d/1jFk0SYh11b5y_pdiZ4OYSAUIy7Udt4NbOJ21UCxh9_g/edit#heading=h.odlsv7xt0st2
namely, the inclusion of account id, request id, and ophid in atlas_logs and unifying the field names

[burov]

Looks almost done,but i still have a few concerns

[burov]

Can we initialize statusCode variable which store value of status.Code(err) instead of calling this function three times

[addudko]

Not my implementation, should be deprecated if not used. Just updated deprecated field

[burov]

I would like to avoid magin strings us DefaultGRPCCodeKey constant instead

[addudko]

Not my implementation, should be deprecated if not used. Just updated deprecated field

[burov]

@Evgeniy-L what do you think about generating request id there, from my point of view it's not a good thing, because we spread request id handling between requestid interceptor, nginx and logging library. From my point of view it better to do the same thing as we did with accountID return an error in case we can't find request id

[Evgeniy-L]

I agree with @burov, we should not spread the logic between multiple parties.
@addudko, please address this comment (e.g. return error similar to account_id).

[burov]

Can we just use this one https://golang.org/pkg/time/#Duration.Milliseconds ?

[burov]

Can use describe why we have this option? I don't see any obvious case where i want to customize translation of duration to miliseconds

[burov]

Can we add the same option to get CustomFields from headers as well, from my point of view it can be reasonable

[marcinas]

With the additional changes implemented that were discussed with architects, this looks good to establish the interceptor side of standard logging 👍
At least from the side of the logging gap analysis and common logging fields, this LGTM as long as the other reviewers' comments are resolved.

[Evgeniy-L]

Please address one comment related to request_id generation and we can merge this PR.

[Evgeniy-L]

I agree with @burov, we should not spread the logic between multiple parties.
@addudko, please address this comment (e.g. return error similar to account_id).

[Evgeniy-L]

Looks good

[kd7lxl]

Many Atlas services use gRPC streams, so a Stream interceptor must also be provided. It seems only Unary was implemented.

[kd7lxl]

It would be helpful if there was a way to pass the logrus global logger here instead of always creating a new logger. This way we could apply these formatter and level settings globally for the process, which will take effect on all packages that use logrus, regardless of whether they accept a logger to their functions.

[kd7lxl]

If this were implemented as a ChainUnaryServer, it could be composed of existing interceptors, like the requestid middleware, without having to reimplement them.