Skip to content

Conversation

@flimzy
Copy link
Contributor

@flimzy flimzy commented Nov 9, 2023

This addresses half of #1401. The easy half, honestly.

I have an implementation of the other half we've been using in our own project, but it will need some polish before making it public, so I'm submitting this PR now to gauge interest of the project maintainers. If we like this, I'll work on polishing the other half in a later PR.

@flimzy
Copy link
Contributor Author

flimzy commented Nov 9, 2023

I see that CI is failing, and probably needs to be updated.

For the tests in this PR to be meaningful, we'll alsoneed to run tests against Go 1.21 (since the new code is behind a build tag).

If you'd like me to create a PR to address any of these, please let me know. I'm quite happy to contribute, but don't want to waste my effort if the package maintainers aren't interested 😉

@thaJeztah
Copy link
Collaborator

(did a quick rebase to make CI run)

Copy link

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@thaJeztah this LGTM, although I added some minor concerns regarding the usage of a logger Vs a handler. It's basically about not swallowing errors.

@flimzy 👋 Happy to support you here and make progress to eventually merge this into the project.

Copy link

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM thanks for addressing my comments!

I'd only extend the comment regarding why runtime.Callers(3, pcs[:]), but not a strong requirement.

@flimzy
Copy link
Contributor Author

flimzy commented Jan 15, 2026

I'd only extend the comment regarding why runtime.Callers(3, pcs[:]), but not a strong requirement.

That was just copied verbatim from the log/slog package's implementation of log(), without considering whether 3 is the correct number of frames to skip... 🤔

@flimzy
Copy link
Contributor Author

flimzy commented Jan 15, 2026

I've added a test to validate that we report the proper caller, and updated the skip argument accordingly.

Comment on lines 72 to 78
attrs := make([]any, 0, len(entry.Data))
for k, v := range entry.Data {
attrs = append(attrs, slog.Any(k, v))
}
var pcs [1]uintptr
// skip 8 callers to get to the original logrus caller
runtime.Callers(8, pcs[:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking if there's some optimisations to be made here; there's also entry.Caller - wondering if that should be used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh - I was lazy; this is what ChatGPT suggested for this; not entirely sure if the runtime.Callers suggestion is good;

	var pc uintptr
	if entry.Caller != nil {
		pc = entry.Caller.PC
	} else {
		var pcs [1]uintptr
		// Skip runtime.Callers + this function; adjust as needed or make configurable.
		runtime.Callers(2, pcs[:])
		pc = pcs[0]
	}

	r := slog.NewRecord(entry.Time, lvl, entry.Message, pc)

	if len(entry.Data) != 0 {
		attrs := make([]slog.Attr, 0, len(entry.Data))
		for k, v := range entry.Data {
			attrs = append(attrs, slog.Any(k, v))
		}
		r.AddAttrs(attrs...)
	}

	return handler.Handle(ctx, r)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely can do that, but it subtly changes behavior. Perhaps in a good way?

If we use entry.Caller.PC, then to get source= in the final output, both Logrus and the slog handler need to be configured to show sources. With the current implementation, the behavior depends only on slog's configuration.

I'm not sure which is "more correct". I suppose in practice, they'll likely both be enabled or both disabled. So I'll make the change to use entry.Caller.PC unless you feel that's not appropriate :)

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.

3 participants