Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(logging): make toLogEntry function public #3863

Merged
merged 2 commits into from Apr 9, 2021

Conversation

@loburm
Copy link
Contributor

@loburm loburm commented Mar 25, 2021

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

return toLogEntryInternal(e, nil, makeParent(parent))
}

func toLogEntryInternal(e Entry, client *Client, parent string) (*logpb.LogEntry, error) {
Copy link
Member

@codyoss codyoss Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@loburm loburm Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@0xSage 0xSage Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@loburm loburm Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@loburm loburm Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@0xSage 0xSage Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
// TODO: Handle error.
}
fmt.Println(le)
Copy link
Contributor

@0xSage 0xSage Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@0xSage 0xSage Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@loburm loburm Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@0xSage 0xSage Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@codyoss codyoss Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.

Copy link
Contributor Author

@loburm loburm Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@0xSage 0xSage Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@loburm loburm Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@0xSage 0xSage Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@loburm loburm Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@0xSage 0xSage commented Apr 8, 2021

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

@codyoss
Copy link
Member

@codyoss codyoss commented Apr 8, 2021

@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
0xSage approved these changes Apr 8, 2021
@0xSage
Copy link
Contributor

@0xSage 0xSage commented Apr 9, 2021

@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.

This will allows clients of the library to easily migrate from using
Logger abstraction to calling WriteLogEntries directly.
@loburm
Copy link
Contributor Author

@loburm loburm commented Apr 9, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants