Added checks for logger not being attached to the entry type and handling gracefully

[sinhashubham95]

No description provided.

[freeformz]

@sinhashubham95 Can you describe the scenarios under which this would occur? And can you add some tests to ensure those scenarios are covered?

[sinhashubham95]

@freeformz the thing is you are exposing the type entry as a result of which anyone can create a new variable of the same without associating it to a logger and attempt to log.
This change will avoid the panic that will happen in such scenarios.
I will add test cases to ensure these scenarios are covered.

[freeformz]

I feel like this shouldn't happen. If no logger is attached, I don't expect any output.

[sinhashubham95]

This is an error scenario. So at least the logs should go to the standard error output. No error should be blindly ignored.

[sinhashubham95]

@freeformz any update, if everything is fine, can you go ahead and merge the pull request.

[freeformz]

@sinhashubham95

Is this "solvable" by adding a disclaimer on the Entry godoc that says "An Entry without a Logger is basically useless and not setting Logger will cause most, if not all of Entry's methods to panic" ?

I'm not personally inclined to merge this for the following reasons:

• I don't feel comfortable adding the additional logging to stderr. There are plenty of situations that I know of where that would break the formatting of a log stream. i.e. Where 2>&1 and stdout is in json format. The log processors down stream will likely throw away at least the json message(s) interfered with as well as the unformatted text to stderr.

• I can think of a bunch of cases in the stdlib where a type is exported and you are expected to use one of its constructors to get a valid value back, but since it's exported you can construct one yourself, but if done improperly may cause a panic.

Perhaps one of the other maintainers feels differently?

[sinhashubham95]

@freeformz what I think is as an API developer or basically a Utility developer, the API or Utility should be full-proof, not depending on some disclaimer, rather it should not panic in any known scenario, rather provide proper logs using which we can get to know the root cause of any issue.

I will tell you a scenario that I faced just today in production. We are using a monitoring tool, but don't have the money enough to buy the support using which for any process crash, I can download the crash dump. At that point in time, I really felt panic in Go is the worst thing to happen to you, as I had basically no clue to know what caused this.

In short, I strongly think the above change is required.

[sinhashubham95]

@freeformz any update on this?