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 Logger.Write method to interface #4091

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Jun 17, 2024

What changed

  • add Write(*LogEntry) to the logger interface

Why

We need a public method that takes a pre-existing LogEntry to do subprocess re-logging in agent. (i.e. to consume, parse, and re-upload log lines without double-prefixing)

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jun 17, 2024
@abe-winter abe-winter marked this pull request as ready for review June 17, 2024 16:39
Copy link
Member

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

I think I'm being looped in a bit late here. Does this have to do with the pattern matching log stuff the agent does?

What functionality is this offering? I would expect user-code that needed access to the structure log object could get that by implementing an appender.

@abe-winter
Copy link
Member Author

abe-winter commented Jun 17, 2024

my understanding is that the current public Logger interface will always format the input (i.e. add a date + level prefix on the front)

problem we are solving: in agent, we are capturing logs from subprocesses, parsing them, and then re-logging them. in theory this is necessary to use netappender without having a 'double prefix' issue

in the current agent PR, there's a global netappender which can receive a LogEntry; this works in theory, but in the spot where I consume the global netappender, I also have access to the logger which contains that netappender. james pointed out that if I could Write(LogEntry) on the logger, we are free + easy

@Otterverse
Copy link
Member

I think I'm being looped in a bit late here. Does this have to do with the pattern matching log stuff the agent does?

What functionality is this offering? I would expect user-code that needed access to the structure log object could get that by implementing an appender.

To clarify, we have structured input already coming in. We want to write it directly, rather than generating new log entries (with potentially different data, like timestamps and filename:lineno combinations.) We want that to be written to the normal logger with the net appender.

In normal "zap" land, this would be done with something like logger.Core().Write(), but as this logging system in RDK was implemented as a wrapper on top of the zap logger, instead of something like a customized zap.Core, so doing that bypasses the appender functionality. This seems to be the next best solution instead, since we're not following the zap paradigm.

Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

LGTM, but should wait until Dan is comfortable too.

@@ -154,7 +154,7 @@ func (imp *impl) shouldLog(logLevel Level) bool {
return logLevel >= imp.level.Get()
}

func (imp *impl) log(entry *LogEntry) {
func (imp *impl) Write(entry *LogEntry) {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this makes sense to me now. Sorry I was being dense.

@@ -16,6 +18,7 @@ type Logger interface {
Sublogger(subname string) Logger
AddAppender(appender Appender)
AsZap() *zap.SugaredLogger
Write(*LogEntry)
Copy link
Member

Choose a reason for hiding this comment

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

No idea why the linter doesn't complain about any of these. Can we explicitly document this with:

Unconditionally logs a LogEntry object. Specifically any configured log level is ignored.

I might follow up with adding a Log(*LogEntry) that conditionally logs. So we can replace this switch statement usage.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jun 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jun 21, 2024
@abe-winter abe-winter merged commit fc56057 into viamrobotics:main Jun 21, 2024
16 checks passed
@abe-winter abe-winter deleted the expose-log-write branch June 21, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants