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

logging: fix caller in echo_logrus and simple logger #4132

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

Conversation

schuellerf
Copy link
Contributor

Simlar to osbuild/image-builder#1176 this fixes the "file" and "func" of the logs

(added newlines for readability)
before

composer-1           | time="2024-05-07T10:27:20Z" level=info msg="Dequeued job"
func="github.com/osbuild/osbuild-composer/internal/common/slogger.(*simpleLogrus).log"
file="/osbuild-composer/internal/common/slogger/logrus.go:29"
job_dependencies="[3c3b52c4-a9d7-4ab0-b4a0-c08fd40b67f2]" job_id=f3ea7917-4bb6-445c-b1e6-bac283222fe6 job_type=manifest-id-only

after:

composer-1           | time="2024-05-07T11:34:33Z" level=info msg="Dequeued job"
func="github.com/osbuild/osbuild-composer/pkg/jobqueue/dbjobqueue.(*DBJobQueue).dequeueMaybe"
file="/osbuild-composer/pkg/jobqueue/dbjobqueue/dbjobqueue.go:421"
job_dependencies="[f3ea7917-4bb6-445c-b1e6-bac283222fe6]" job_id=3a2e0767-3fc0-4138-a4b0-28f2cf7c1366 job_type="osbuild:x86_64"

@bcl
Copy link
Contributor

bcl commented May 8, 2024

I think some of this could be simplified by making the ctxHook bits public in the common package and use it from there in slogger instead of duplicating it. I'd also call fixCaller logWithCaller instead, but that's just me:)

@lzap
Copy link
Contributor

lzap commented May 14, 2024

This is still a bit too complicated. Why don’t you simply simply check and cast logrus.Logger to logrus.Event and set event.Caller directly. No need to pass things around with context.

For a cleaner approach, you can turn off caller reporting globally and then just set event.Caller in a hook, but there is no need to capture the caller and pass it via context. It seems that events are fired right away log entry is processed by the main log method, that is just few frames down the stack. You just need to tune the 3 number to something else like 5 and that should do it.

Also what can bite later on is that the code sets the hook only on the standard logger, but new instances of the struct can be created freely with totally different loggers which may not have the event there. For this reason, I really think this hook should be defined globally at some different place, not in these types.

If it turns out that it is much easier to capture the caller earlier, I suggest to avoid using context for that and simply storing the data in a new field that can be processed and removed at the end of the pipeline in the hook. Fields are just a map, so removing should be safe.

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.

None yet

3 participants