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

Fix caller in logs #1176

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

schuellerf
Copy link
Contributor

@schuellerf schuellerf commented May 7, 2024

@lzap based on your #1174 I'd like to add the fix to show the correct caller (for the cases using echo_logrus.go)
I'd keep this as draft until yours is merged

Do you think this makes sense?

example log message is (just added some newlines for readabililaty)
before:

backend-1            | time="2024-05-07T09:08:36Z" level=error msg="RHEL entitlement not present in identity header"
func="github.com/osbuild/image-builder/internal/common.(*EchoLogrusLogger).Error"
file="/go/src/github.com/osbuild/image-builder/internal/common/echo_logrus.go:153"
distribution=centos-9 insights_id=eIBJqJJqkaz7 method=GET path="/api/image-builder/v1/architectures/:distribution" request_id=FbaXzyS1epxn

after:

backend-1            | time="2024-05-07T09:12:52Z" level=error msg="RHEL entitlement not present in identity header"
func="github.com/osbuild/image-builder/internal/v1.(*Identity).IsEntitled"
file="/go/src/github.com/osbuild/image-builder/internal/v1/server_identity.go:76"
distribution=centos-9 insights_id=XMckxfNStVNf method=GET path="/api/image-builder/v1/architectures/:distribution" request_id=yIP6lO4O6wmG

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>
@schuellerf schuellerf requested review from lzap and croissanne May 7, 2024 09:28
}
if e.Context.Value(common.LoggingFrameCtx) != nil {
frame := e.Context.Value(common.LoggingFrameCtx).(runtime.Frame)
e.Caller = &frame
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not the golang expert - is this safe to return this pointer?
although the implementation of logrus does the same - it feels bad :-|


const (
LoggingFrameCtx ctxKey = iota
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a good practice not to export context key types and values so other packages cannot modify the values. Here it is a mix - you export the value but not the type.

@lzap
Copy link
Collaborator

lzap commented May 10, 2024

Yeah I noticed this as well. Tho the implementation seems fragile, creating a context value per request is fine, but per log entry? There can be many log entries per request, this could get out of control. Cannot you modify the caller on the fly directly? Or even better - turn off automatic caller adding and implement your own hook that will consider this wrapper instead.

Whatever you do, please make a unit test for this, do some logging directly through logrus as well as through the echo facade to ensure consistency.

@schuellerf
Copy link
Contributor Author

Note: related implementation: osbuild/osbuild-composer#4132

Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants