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.

[codyoss]

If we think there is a big enough use case for something like this I wonder if we should make an optional Parent field on Entry? It could fall back to the parent specified on the client if not set.

[loburm]

Afraid that it can be a little bit dangerous. Unless you think that's completely OK to fail ToLogEntry if we need this field for traceID?

[0xSage]

Entry already has parent as a substring in its LogName field.

Might I suggest creating ToLogEntry as a method on Entry:
func (e *Entry) ToLogEntry(client ...*Client). It's still not dependent on client like you wanted. And function signature is cleaner, and we dont need to create new internal functions.

Unless, we dont like using variadic parameters to enable optional parameters in Go @codyoss ?

[loburm]

LogName can't be used, because toLogEntry is failing whenever we set that field. Unless you want me to remove that check?

[loburm]

Strange, I have received a comment, but don't see it here.

I think it makes sense to remove that check in cases where there's no client/logger. And to update the corresponding comments. Ofc when there is a logger/client, then users can't set the logname (same behavior as before).
Also LogName and Resource fields are required in order to write to Cloud Logging. As a part of your implementation, can you also write checks to ensure that if ppl use ToLogEntry, they are creating valid LogEntries?

1. (Related to previous comment). Golang doesn't support optional arguments. client ...*Client means that we can pass multiple clients and that's a very confusing in this context.
2. I think different behavior when you call with Client and without is also very confusing.
3. LogName and resource are optional. They can be set on WriteLogEntries level or even ignored completely.

[0xSage]

I deleted this comment because after thinking through various modifications to make this PR work, I don't think this logic should be introduced at the client library at this veneer layer. Agree that using a variadic param to hack things isn't the right way to go.

[0xSage]

If users still want to write to Cloud Logging (rather than wait for an agents to pick up stdout logs) , how might they do that?

[0xSage]

The main UX concern I have with this PR is we're exporting a function within the manual layer of the client library, that only lets users write to stdout (unless we introduce another set of log/logsync functions). It seems there's no elegant way to hack the current manual library layer to behave at a lower level.

Can you work with the API (lower client) directly, rather than modifying the manual layer to work at the API level? i.e. copy and paste the toLogEntry fn within your own implementation

What's the rationale for depending on this manual layer of the library? I believe Cody also suggested this in your previous PR

[loburm]

I have extended this example to show how this function can be used for sending logs. Exporting this function in this layer makes most sense, because Entry object is defined here. Of course customers can create LogEntry proto directly and do not depend on Entry at all and in such case there is no need in exposing any new function. But for projects that have been relying on Entry and are unhappy with Logger this is an easy way to use WriteLogEntries with minimal amount of code changes.

[0xSage]

My only concern with this: We're adding a function into the logging module, that can only be used with functions from the v2 module.

// In the example: 
                       // logging module
le, err := logging.ToLogEntry(e, "my-project")
...
                       // v2 module
	client, err := vkit.NewClient(context.Background())
	...
	_, err = client.WriteLogEntries(context.Background(), &logpb.WriteLogEntriesRequest{
		Entries: []*logpb.LogEntry{le},
		LogName: "stdout",
	})

And I don't believe this is a common use case for external users.

It seems better practice for customers who need this modification, to just directly implement a custom Entry on top of v2 module, rather than modify this higher level module to work with a lower level module. Less overhead.

@loburm what's the challenge with directly implementing your own Entry/Logger on v2 module?

We can still merge this PR as a solution, but it would just be for your unique use case.

[codyoss]

... just directly implement a custom Entry on top of v2 module, rather than modify this higher level module to work with a lower level module

I agree. I think the example code here is a great example.

[loburm]

OK, so you basically propose to copy paste Entry and toLogEntry function (and all other helper functions) to v2 module? This is going to create near 150 lines of duplicated code and will increase maintenance cost, because if any issue will be found in one of them, then it would be great to also fix it in another place.

[0xSage]

I mean: Can you implement Entry and toLogEntry on your own, in your own codebase? Essentially create your own custom local module, which depends on v2 module.

We actually can't edit v2 module because it's autogenerated. Our changes to it would be overridden.

[loburm]

So a use case for this function is to support customers that want to migrate away from Logger to something more low level (vkit.Client in this case). Implementing it in our own codebase means that we need to copy all the code and monitor for any bug fixes and/or new features.

Another use cases might be for other customers that don't want:

1. Think about how to extract all the data from HttpRequest structure.
2. Think about how to convert text or structured payload to the correctly nested structure inside of LogEntry.

[0xSage]

I'm not convinced it's a huge use case for external customers. However, I trust that you know best on where to implement logic for agents. Since agents are a big use case for this library, I can LGTM this after tests pass.

Can you change your ToLogEntry comment to (1) warn users that this feature is a lower level feature, isn't compatible with Log/LogSync, and (2) add a developer comment the fact this feature is necessary to support agents.

Parting thoughts:

1. Doesn't relying on this library add overhead (latency and size) for your agents instrumentation? Over directly implementing your own custom logic & calling the API endpoints.
2. In the future, we should refactor this logic at the log/logSync step instead of ToLogEntry, such that users can quickly switch back and forth between writing to STDOUT vs Cloud Logging. And at your level, you can just override the logname/fields written to stdout or override log/logsync.

I instrumented something similar in Node for a hackathon. This is better UX, as users wouldn't have to use 2 separate libraries to write logs to Cloud Logging, in addition to stdout.

[loburm]

Nicole, thanks for the review. I have updated comments as it was suggested by you.

1. Doesn't relying on this library add overhead (latency and size) for your agents instrumentation? Over directly implementing your own custom logic & calling the API endpoints.

Right now we haven't experienced any problems with using Entries. Our agent might get some benefits from creating LogEntry directly, but I don't expect it to be significant gain.

[0xSage]

@codyoss how do we trigger tests to run on this PR? Do we need to LGTM it first?

[codyoss]

@nicoleczhu The kokoro tag should do the trick. If a PR gets updated the tag need to be readded, only good for one commit.

[0xSage]

@loburm I lgtm'd , can you remove logging/logging.iml file before merging? its a ide thing we dont need right?
Remove the do not merge label after removing the file, and you should be able to merge this.

[loburm]

Marian Lobur I lgtm'd , can you remove logging/logging.iml file before merging? its a ide thing we dont need right?
Remove the do not merge label after removing the file, and you should be able to merge this.

@nicoleczhu thanks a lot. Sorry, it seems that logging.iml was added accidentally and now removed. Unfortunately I don't have a permission to remove that label. Could you do it instead?