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

Add WithCaller and WithCallerAt for overriding the caller detection #1215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nelsonxb
Copy link

Seems like there's already a few PRs along these lines (e.g. #973, #1175)... but I forgot to check for those before implementing this 😅

Anyway, I think this approach should be more flexible and useful. I don't think the other approaches would quite work for my purposes... or at least, they'd be incredibly awkward.

The new methods Event.WithCaller, Logger.WithCaller allow for setting the caller with an existing frame, and exists mostly as a Just-In-Case™. The other new methods Event.WithCallerAt, Logger.WithCallerAt are the main source of interest (at least for me), and make it simple to log a message attached to the caller of the function that needs to log. For example:

func DoSomething() error {
  // ...
  handleResult(result, err)
}

func handleResult(result string, err error) {
  if err != nil {
    // ...
    logger.WithCallerAt(1, err) // The error happened in the calling function
  }

  // ...
}

@Sytten
Copy link

Sytten commented Feb 6, 2021

@sirupsen Can this be added, it is quite common when writing adapters that the caller is just wrong and currently it gets overriden if the adapter tries to set it manually.

@nelsonxb
Copy link
Author

nelsonxb commented Mar 18, 2021

Honestly, I doubt that this PR will get merged, as upstream logrus isn’t accepting new features. Though it’d be great if this PR could be left open regardless, so those who do need this feature can find it.

If you do wish to use my fork, you can still use the import of github.com/sirupsen/logrus, by adding a replace statement to your go.mod file. This won’t work very well if you’re a library expecting to be imported, but it might help some users if they want it that badly.

In the meantime, I’ll be updating my fork tonight to the latest revision of upstream; and I’ll try to keep doing that every now and then. Logrus doesn’t change much (likely due to the whole maintenance mode thing), so it shouldn’t be too far out of date at any given time.

Also ensured that the Entry.Caller will be persisted through other
Entry.With* methods.
@shallowclouds
Copy link

This is what I need!

But I think passing caller via Entry is enough, it should be avoided to imported new methods for Entry as logrus is not developing actively at present.

@nelsonxb
Copy link
Author

If any of the maintainers confirm that removing the WithCaller methods would get this merged, I’ll happily oblige.

But failing that, I feel that particularly WithCallerAt is far too useful to drop out-of-hand. Not only does it just do nothing if we don’t want the performance penalty, but it allows a caller to not have to fiddle around with reflect for a relatively simple use case.

@edoger
Copy link
Contributor

edoger commented May 18, 2021

I really hope this feature can be merged! Feeling sleepy.

@Lvzhenqian
Copy link

One year has passed, is there any news about this merger? :(

@LAShZ
Copy link

LAShZ commented Aug 28, 2022

Is there any message about this commit or other similar ones? It's seems that I still have the problem of setting caller depths when use a wrapper of logrus :<

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

Successfully merging this pull request may close these issues.

6 participants