Logger interface

[kruftmeister]

Hi, since I saw you're open to PRs, I'd like to propose this refactoring of the logger from a very specific implementation into a global exported variable implementing a minimalistic interface. The reasons for the proposed change include the following:

1. This being a library to be integrated in applications, would leave the choice to the developers of the application using the sensor, whether to enable the sensor logging (by setting instana.Logger) or not (the default logger in the PR discards all messages) and on which log level to do so.
2. I'd argue the point that most applications use structured loggers, so they could set the logger variable to their logger or write a small adapter that implements the three methods declared in StdLogger.
3. Not everyone logs messages to StdOut as the default logger does.

[pavlobaron]

hi @manolovl ,

thanks a lot for the PR! Before we move forward with it, there is some things we should consider:

1. the idea of the original implementation was to be able to separate debug, error and info log in the sensor code. Does that mean it's now moved behind the StdLogger interface? How would the target implementation be able to tell apart debug from info? The sensor is pretty chatty when on debug log level.

2. once clarified, the README should be adjusted, too. I assume, I think it's just about dropping the log-level in the opts.

cheers pb

[kruftmeister]

Hi @pavlobaron,

I see your point about separation of log levels, but frankly I wouldn't mix up sensor logs in the logs of the application, using it. I'd like have the instana.Logger instantiated, the way it is done per default and if I realise I don't see traces for an application I'd go and "turn it on" to find out why, by looking into all sorts sensor's log entries. These would all be "debug" or "trace" level for me - I'd implement an adapter for my logger so that all the methods, declared in the StdLogger, output at the said log level. A prefix in the log messages (just got the idea :) ) would then help me distinguish between log entries from my application and the sensor.

Cheers,
Lyuben

[kruftmeister]

I also just edited the README, in case the PR would be approved.

[pavlobaron]

hi @manolovl ,

thanks, I thought about it. I don't think separating log level by string patterns solves what I mean, but I also get your points. What I'm thinking of is providing "mode" instead of log level in the opts when instantiating sensor/tracer, so I can use it not to debug-log if in "prod" mode, or only debug-log when in "dev" mode. This solves what you're after and what I was thinking of. WDYT?

cheers pb

[pglombardo]

In our other instrumented languages we just log a boot message and if everything goes well, we never print another thing. In case of error, we log a single message and handle the best we can. If not, we go sit in the corner and just not do anything.

Other common requirements are:

• default to stdout/stderr but can be replaced/overridden by the application (like you suggest in this PR)
• messages prefixed with a label such as [instana]
• configurable via env variables for dev & investigating customer issues/environments

So good points on both sides and it's the right idea but I think it would be best if we stay consistent with our other languages.

It seems we may have to use a 3rd party logger or roll our own to meet the above.

BTW it seems that there is a proposal to augment StdLogger that also goes into metrics and traces slightly:
https://groups.google.com/forum/#!topic/golang-dev/F3l9Iz1JX4g

@manolovl I believe we are going to have to pass on this specific PR but I agree logging needs to be fixed in this package. I'd love to hear any other ideas/concerns/input you have on this.

[pglombardo]

Filed #13 in response to this PR with requirements. Closing this PR. If any objections/concerns, please let me know. Thanks @manolovl !

[kruftmeister]

No, not all. Meanwhile I also came to the conclusion that there would be good if to have a dedicated (and als default) logger, implementing certain interface, i.e. the one of Logrus, so it could be then swapped by the one, used in the env, where the sensor is being used.

[pglombardo]

+1 on Logrus - looks really slick.