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

echo_logrus: use std context for fields #1174

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Conversation

lzap
Copy link
Collaborator

@lzap lzap commented May 6, 2024

This is a followup of https://github.com/osbuild/image-builder/pull/1171/files

I have couple of concerns with the recent changes.

  • The new fields are only present when echo logger is used. There are many other logs created through logrus interface (e.g. all SQL queries done by the pgx driver) which will miss all those fields.
  • All path params are added and can possibly override existing fields e.g. msg or level. This could have unwanted effects, this new patch protects the most important fields. (I haven’t tested this chances are logrus would not allow overriding level or msg fields at all.)
  • There were concerns about memory consumption or performance - logrus.Fields is a map and context is a lightweight object by design. Paths typically contain one or two fields maximum, with path and method this is up to 4 map elements.
  • Context will carry a copy of information, that is a valid point (see the original PR discussion), but as described above this is very small amount of information.
  • I noticed context was not added in "Finished request" message, it is now added.

I have to add I do not believe it is a good thing to add path, method and all path params to every single log line, I think it would be better to only log this once at the beginning of the request and devs can use correlation ids to find the line from any logged line. This proposal adds those fields everywhere tho.

})

if !SkipPath(c.Path()) {
c.Logger().Debugf("Started request")
ctxLogger.WithContext(ctx).WithFields(rd).Debugf("Started request")
Copy link
Collaborator Author

@lzap lzap May 6, 2024

Choose a reason for hiding this comment

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

This hunk is probably unnecessary, I was experimenting around to find the best solution. Can put it back, should work just fine. It really does not matter if you use echo interface or logrus interface, the result is the same. In fact, the counterpart for this line "Processed request ..." does use logrus interface, so why not to keep this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctxLogger.WithContext(ctx).WithFields(rd).Debugf("Started request")
ctxLogger.WithContext(ctx).Debugf("Started request")

my WithFields is also not needed here, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is either this, or I can make use of the echo logger created above. Let me use the echo logger instead, looks better.

@@ -154,7 +154,8 @@ func main() {
if values.Error != nil {
fields["error"] = values.Error
}
logrus.WithFields(fields).Infof("Processed request %s %s", values.Method, values.URI)
logrus.WithContext(c.Request().Context()).
WithFields(fields).Infof("Processed request %s %s", values.Method, values.URI)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here WithContext was missing, somewhat unrelated to the change but I think it is worth fixing.

Copy link
Contributor

@schuellerf schuellerf May 13, 2024

Choose a reason for hiding this comment

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

do you think also adding
"path": c.Path(),
to the fields here would be beneficial as this is better for log entry searches/grouping (in contrast to values.URI) I guess
but no stopper for this PR I guess
(hmm or is this added by the c.Request().Context() anyway? - I'll check)

}

func (l *EchoLogrusLogger) Panicj(j log.JSON) {
b, err := json.Marshal(j)
if err != nil {
panic(err)
}
l.Logger.WithContext(l.Ctx).WithFields(l.Fields).Panicln(string(b))
l.Logger.WithContext(l.Ctx).Panicln(string(b))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is a full revert. No other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome thanks, looks nicer and even covers more use cases!

Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

Very nice, thanks

})

if !SkipPath(c.Path()) {
c.Logger().Debugf("Started request")
ctxLogger.WithContext(ctx).WithFields(rd).Debugf("Started request")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctxLogger.WithContext(ctx).WithFields(rd).Debugf("Started request")
ctxLogger.WithContext(ctx).Debugf("Started request")

my WithFields is also not needed here, right?

}

func (l *EchoLogrusLogger) Panicj(j log.JSON) {
b, err := json.Marshal(j)
if err != nil {
panic(err)
}
l.Logger.WithContext(l.Ctx).WithFields(l.Fields).Panicln(string(b))
l.Logger.WithContext(l.Ctx).Panicln(string(b))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome thanks, looks nicer and even covers more use cases!

@lzap
Copy link
Collaborator Author

lzap commented May 10, 2024

Rebased.

Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -154,7 +154,8 @@ func main() {
if values.Error != nil {
fields["error"] = values.Error
}
logrus.WithFields(fields).Infof("Processed request %s %s", values.Method, values.URI)
logrus.WithContext(c.Request().Context()).
WithFields(fields).Infof("Processed request %s %s", values.Method, values.URI)
Copy link
Contributor

@schuellerf schuellerf May 13, 2024

Choose a reason for hiding this comment

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

do you think also adding
"path": c.Path(),
to the fields here would be beneficial as this is better for log entry searches/grouping (in contrast to values.URI) I guess
but no stopper for this PR I guess
(hmm or is this added by the c.Request().Context() anyway? - I'll check)

@lzap
Copy link
Collaborator Author

lzap commented May 14, 2024

The way I search paths is simply doing normal search across all fields and once I get request-id I search for that to get the full history of a request. This will work because the path is logged at least once on every request. I can add this if you want.

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 14, 2024
Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>
@ezr-ondrej
Copy link
Collaborator

/retest

@schuellerf schuellerf merged commit fb712d0 into osbuild:main Jun 19, 2024
12 checks passed
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