feat(logging): make toLogEntry function public

[loburm]

This will allows clients of the library to easily migrate from using
Logger abstraction to calling WriteLogEntries directly.

[freelerobot]

From a UX perspective, these changes are helpful.

The only slightly weird behavior that's introduced now is that the e.HTTPRequest conversion will only write errors to the logger's error channel, when users might expect the function to directly return the error. Not the end of world.

[freelerobot]

(1) Expand the fn comment to the effect of "ToLogEntry is implied when users invoke Logger.Log or Logger.LogSync, but its exported as a pub function here to give users additional flexibility when using the library". This is so majority users don't feel like they have to do the conversion themselves.

[freelerobot]

(2) Add an example in examples_test.go. Name it: ExampleLogger_ToLogEntry

Note: doc and example additions will be autogenerated here.

[codyoss]

Is there an issue for this for discussion? Here are couple of my thoughts:

• I don't think any of our other manual clients make these conversions functions exported, why should this library be different?
• The point of the manual layer is to provide a more ergonomic experience than working with the generated layer directly. I am not sure if a function promoting the use of the generated layer is the right way to solve this problem.
• What is your use case for wanting to use WriteLogEntries directly instead of the manual layer? Is there a feature missing with this library?

[freelerobot]

Is there an issue for this for discussion? Here are couple of my thoughts:

• I don't think any of our other manual clients make these conversions functions exported, why should this library be different?
• The point of the manual layer is to provide a more ergonomic experience than working with the generated layer directly. I am not sure if a function promoting the use of the generated layer is the right way to solve this problem.
• What is your use case for wanting to use WriteLogEntries directly instead of the manual layer? Is there a feature missing with this library?

I'll let @loburm elaborate here. My understanding is that their implementation of GKE logging agent uses this lib. And certain abstractions we make around logger/logname/batching, are too heavyhanded for their use case. They need a middle ground btw raw API and current library exports, namely the tologentry private function.

Will defer to @codyoss on best practices here. I like the idea of letting users have access to different levels of abstraction, but will definitely acknowledge our internal best practices.

[loburm]

@nicoleczhu actually I think it makes more sense to refactor logic to remove dependency on client completely. Also after going through it once again it's better not to depend on Logger at all. I'll try to resolve these issues.

@codyoss thanks for taking a look, let me try to explain why our team is unhappy with how Logger works:

1. Logger doesn't allow to overwrite logName for each entry. It basically meas if we have a different logName for each log entry we need to create bunch of loggers. But that also means higher amount of Cloud Logging API calls. We want to avoid it.
2. We can't perform retries reliably. Our team sets relatively low timeout for requests (10 seconds). But when we are unlucky and request takes more than 10 seconds then it's not retried in WriteLogEntries. So we want to add one more retry logic around that method. In case of Logger we will need to Log each entry once again and call Flush method one more time.
3. Log() method doesn't return any error. We don't know why which log entry is dropped (overwriting onError function doesn't help).
4. If Flush method is called from two goroutines and one call is failing, then we don't know how many log entries have been delivered or dropped. Also in logging: race condition in error reporting #3720 I have shown that returned error might come from different logger.
5. Internal errors are retried till timeout and as a result wrong error message is returned. See issue logging: Propagate better errors #3629
6. If I understood correctly how bundler works - it drops new log entries if final size is above the limit. We would prefer to Flush it and then continue adding new entries.

[freelerobot]

@nicoleczhu actually I think it makes more sense to refactor logic to remove dependency on client completely. Also after going through it once again it's better not to depend on Logger at all. I'll try to resolve these issues.

@codyoss thanks for taking a look, let me try to explain why our team is unhappy with how Logger works:

1. Logger doesn't allow to overwrite logName for each entry. It basically meas if we have a different logName for each log entry we need to create bunch of loggers. But that also means higher amount of Cloud Logging API calls. We want to avoid it.
2. We can't perform retries reliably. Our team sets relatively low timeout for requests (10 seconds). But when we are unlucky and request takes more than 10 seconds then it's not retried in WriteLogEntries. So we want to add one more retry logic around that method. In case of Logger we will need to Log each entry once again and call Flush method one more time.
3. Log() method doesn't return any error. We don't know why which log entry is dropped (overwriting onError function doesn't help).
4. If Flush method is called from two goroutines and one call is failing, then we don't know how many log entries have been delivered or dropped. Also in logging: race condition in error reporting #3720 I have shown that returned error might come from different logger.
5. Internal errors are retried till timeout and as a result wrong error message is returned. See issue logging: Propagate better errors #3629
6. If I understood correctly how bundler works - it drops new log entries if final size is above the limit. We would prefer to Flush it and then continue adding new entries.

SGTM, I think that was my initial suggestion in our chat as well. Whichever path works best for your needs.

Closing this one for now

[codyoss]

@loburm Thanks, I think that is some great feedback for the client. I agree, it sounds like your use case would be better solved by working with the lower level client directly.