Arbitrary context call for riverlog middleware
#919
Conversation
b7d08d0 to
1ae432a
Compare
There was a problem hiding this comment.
Seems like a good solution to #914. The only thing I'm not sure about is the "Arbitrary" suffix on this and I'm trying to think of a better one 🤔 Best I've come up with is NewMIddlewareConstructor or NewMiddlewareWithConstructor or NewMiddlewareCustomContext but I don't like any of those much either 🤷♂️
| // out of work context. | ||
| type ArbitraryContextKey struct{} | ||
|
|
||
| type ArbitraryLogginArgs struct{} |
There was a problem hiding this comment.
| type ArbitraryLogginArgs struct{} | |
| type ArbitraryLoggingArgs struct{} |
Maybe we also want to keep the key unexported per typical context conventions?
There was a problem hiding this comment.
Struggled a bit on this one because it felt weird to not export ArbitraryContextKey if ArbitraryLogginArgs right below it is exported, and I didn't want to unexport that because all the other example tests export theirs.
It doesn't matter that much though. I made arbitraryContextKey unexported.
| @@ -145,5 +181,5 @@ func (m *Middleware) Work(ctx context.Context, job *rivertype.JobRow, doInner fu | |||
| metadataUpdates[metadataKey] = json.RawMessage(allLogDataBytes) | |||
There was a problem hiding this comment.
Just realized we're storing a log here even if logData is empty. Do you think that's the right call, or should we skip touching that metadata if there's nothing useful to write?
There was a problem hiding this comment.
Yeah, good call. Fixed that up, added a test case, changelog, and modified the commit message to mention the change.
1ae432a to
8224749
Compare
This one's inspired by #914. Our current `riverlog` implementation should work pretty well for most users, but it does force them to use slog. I'm inclined to provide additional options here because in all honesty, slog's API is terrible, and I wouldn't want to use it myself. Here, we add the new initializer `NewMiddlewareCustomContext`. Users will have to bring their own context key like: type CustomContextKey struct{} New middleware takes a `newContext` function that should initialize whatever it wants with an input writer and put it in context: riverlog.NewMiddlewareCustomContext(func(ctx context.Context, w io.Writer) context.Context { logger := log.New(w, "", 0) return context.WithValue(ctx, CustomContextKey{}, logger) }, nil), Work functions then re-extract whatever they added to context (in reality, they'd probably define their own shorthand helpers over what's shown below): func (w *CustomContextLoggingWorker) Work(ctx context.Context, job *river.Job[CustomContextLogginArgs]) error { logger := ctx.Value(CustomContextKey{}).(*log.Logger) logger.Printf("Raw log from worker") return nil } An alternative to this might be to expose more River innards that'd let callers set their own metadata, but I'm kind of thinking that a project like that should involve very thorough consideration, and may not even be desirable. I think that even if we go that direction, the new `NewMiddlewareCustomContext` would continue to be useful as a higher level abstraction. We also patch up another small `riverlog` problem point out in review in that previously, we'd store a metadata value even if no logs at all were emitted during the course of a work function. Here, we put in a check so that metadata is left unchanged if no logging occurred. A new test case verifies the behavior. Fixes #914.
8224749 to
13b3cad
Compare
Yeah I wasn't crazy about it either but I was having a ton of trouble coming up with anything good that wasn't overly verbose. I changed it to |
|
Thanks! |
This one's inspired by #914. Our current
riverlogimplementationshould work pretty well for most users, but it does force them to use
slog. I'm inclined to provide additional options here because in all
honesty, slog's API is terrible, and I wouldn't want to use it myself.
Here, we add the new initializer
NewMiddlewareCustomContext. Userswill have to bring their own context key like:
New middleware takes a
newContextfunction that should initializewhatever it wants with an input writer and put it in context:
Work functions then re-extract whatever they added to context (in
reality, they'd probably define their own shorthand helpers over what's
shown below):
An alternative to this might be to expose more River innards that'd let
callers set their own metadata, but I'm kind of thinking that a project
like that should involve very thorough consideration, and may not even
be desirable. I think that even if we go that direction, the new
NewMiddlewareCustomContextwould continue to be useful as a higherlevel abstraction.
We also patch up another small
riverlogproblem point out in review inthat previously, we'd store a metadata value even if no logs at all were
emitted during the course of a work function. Here, we put in a check so
that metadata is left unchanged if no logging occurred. A new test case
verifies the behavior.
Fixes #914.