default user logs to send to fn stderr at INFO level #1479
Conversation
rdallman
force-pushed
the
default-logs-hotwire
branch
from
f038c06
to
bdbffdd
Compare
we have WithLogger already, we just need to use it. this moves the config of the logger itself up to be a call option which makes it less brittle in GetCall to hit the path where it might get accidentally turned on. removes the old buffer and io.Reader on stderr so that we could upload logs, we only need the logrus logger for now and it was a bit of a tangled mess. TODO need to add config bit to set the default level to 'info' so that we get logs out of these guys in the agent config (pure runner is left alone to off), with ability to change to debug or turn off altogether. NOTE: this fixes #1328 by putting a write guard after close and not using the line writer for different writers
rdallman
force-pushed
the
default-logs-hotwire
branch
from
64dcd4a
to
dbc2c16
Compare
api/agent/func_logger.go
Outdated
| @@ -148,8 +120,9 @@ func (li *lineWriter) Write(ogb []byte) (int, error) { | |||
| // write in this line and advance buffer past it | |||
| l := b[:i+1] | |||
| ns, err := li.w.Write(l) | |||
| n += ns | |||
There was a problem hiding this comment.
Write() should return "how much of the original buffer was consumed". Looks like existing bug... In other words, in this state, Write() may return n, which can be larger than len(ogb) which is not really what we want to return.. I think we might want to change a few things such as:
- li.b buffer should be limited. Currently, series of Write() calls without a newline can grow li.b.buffer without limits with each Write() having to parse entire buf for newlines.
- if buffer is at limit (or going to grow over limit with current call to Write()), then we should flush li.b with appended newline ourselves before processing ogb. If that flush fails, then we can return error.
There was a problem hiding this comment.
i guess, in practice, the reality of this is that the inner writer is never going to error (it's a logWriter, which never errors) and writing to/reading from the buffer is never going to error either (we're not setting a capacity on buffers, and many methods of the buffer contract will never error - the runtime will oom before an error gets returned, but this is a panic). it's also the case that this is not to be used for production applications and we can avoid thinking about some of the more fun attack vectors here, but would be happy to note them - I just don't think we need to add any more arbitrary configuration than we already have for something that has a configuration to just turn it off for prod apps (glad to fix correctness issues, of course).
that being said, appreciate the feedback can spend an hour here tidying this up so that it's more obviously correct.
api/agent/func_logger.go
Outdated
| if err != nil { | ||
| return ns, err | ||
| return n, err | ||
| } | ||
| li.b.Next(len(l)) |
There was a problem hiding this comment.
this also looks odd. if li.w.Write() return error with partial write, then we do need to execute li.b.Next(ns) regardless of error, no?
closes #1474
see linked issue above for some details of the saga. this patch does a number of things in order to accomplish the goal of sending user logs to fn stderr at INFO level, while allowing it to easily be configured to off. as pure_runner is explicitly setting these to off for the moment, that configuration is not affected by this patch, however in the future it's possible we may want to change these to align so that lb/runner configs can get logs out of user containers in this manner. this only affects the full node (not just full agent) configuration, where calls are created in the invoke/httptrigger handlers from OSS fn. open to discussion on default behavior, however I think we have to have logs by default in some capacity for OSS, absolutely want to make it not brittle and easily disabled for production services. attempt to cover surface area here:
logger.Log(level, stuff)methodtested and she seems to work as expected