parent
267e0f21d8
commit
e6501d254f
@ -0,0 +1,223 @@ |
|||||||
|
--- |
||||||
|
title: >- |
||||||
|
Old Code, New Ideas |
||||||
|
description: >- |
||||||
|
Looking back at my old code with bemusement and horror. |
||||||
|
--- |
||||||
|
|
||||||
|
About 3 years ago I put a lot of effort into a set of golang packages called |
||||||
|
[mediocre-go-lib][mediocre-go-lib]. The idea was to create a framework around |
||||||
|
the ideas I had laid out in [this blog post][program-structure] around the |
||||||
|
structure and composability of programs. What I found in using the framework was |
||||||
|
that it was quite bulky, not fully thought out, and ultimately difficult for |
||||||
|
anyone but me to use. So.... a typical framework then. |
||||||
|
|
||||||
|
My ideas about program structure haven't changed a ton since then, but my ideas |
||||||
|
around the patterns which enable that structure have simplified dramatically |
||||||
|
(see [my more recent post][component-oriented] for more on that). So in that |
||||||
|
spirit I've decided to cut a `v2` branch of `mediocre-go-lib` and start trimming |
||||||
|
the fat. |
||||||
|
|
||||||
|
This is going to be an exercise both in deleting old code (very fun) and |
||||||
|
re-examining old code which I used to think was good but now know is bad (even |
||||||
|
more fun), and I've been looking forward to it for some time. |
||||||
|
|
||||||
|
[mediocre-go-lib]: https://github.com/mediocregopher/mediocre-go-lib |
||||||
|
[program-structure]: {% post_url 2019-08-02-program-structure-and-composability %} |
||||||
|
[component-oriented]: {% post_url 2020-11-16-component-oriented-programming %} |
||||||
|
|
||||||
|
## mcmp, mctx |
||||||
|
|
||||||
|
The two foundational pieces of `mediocre-go-lib` are the `mcmp` and `mctx` |
||||||
|
packages. `mcmp` primarily deals with its [mcmp.Component][component] type, |
||||||
|
which is a key/value store which can be used by other packages to store and |
||||||
|
retrieve component-level information. Each `mcmp.Component` exists as a node in |
||||||
|
a tree of `mcmp.Component`s, and these form the structure of a program. |
||||||
|
`mcmp.Component` is able to provide information about its place in that tree as |
||||||
|
well (i.e. its path, parents, children, etc...). |
||||||
|
|
||||||
|
If this sounds cumbersome and of questionable utility that's because it is. It's |
||||||
|
also not even correct, because a component in a program exists in a DAG, not a |
||||||
|
tree. Moreover, each component can keep track of whatever data it needs for |
||||||
|
itself using typed fields on a struct. Pretty much all other packages in |
||||||
|
`mediocre-go-lib` depend on `mcmp` to function, but they don't _need_ to, I just |
||||||
|
designed it that way. |
||||||
|
|
||||||
|
So my plan of attack is going to be to delete `mcmp` completely, and repair all |
||||||
|
the other packages. |
||||||
|
|
||||||
|
The other foundational piece of `mediocre-go-lib` is [mctx][mctx]. Where `mcmp` |
||||||
|
dealt with arbitrary key/value storage on the component level, `mctx` deals with |
||||||
|
it on the contextual level, where each go-routine (i.e. thread) corresponds to a |
||||||
|
`context.Context`. The primary function of `mctx` is this one: |
||||||
|
|
||||||
|
```go |
||||||
|
// Annotate takes in one or more key/value pairs (kvs' length must be even) and |
||||||
|
// returns a Context carrying them. |
||||||
|
func Annotate(ctx context.Context, kvs ...interface{}) context.Context |
||||||
|
``` |
||||||
|
|
||||||
|
I'm inclined to keep this around for now because it will be useful for logging, |
||||||
|
but there's one change I'd like to make to it. In its current form the value of |
||||||
|
every key/value pair must already exist before being used to annotate the |
||||||
|
`context.Context`, but this can be cumbersome in cases where the data you'd want |
||||||
|
to annotate is quite hefty to generate but also not necessarily going to be |
||||||
|
used. I'd like to have the option to make annotating occur lazily. For this I |
||||||
|
add an `Annotator` interface and a `WithAnnotator` function which takes it as an |
||||||
|
argument, as well as some internal refactoring to make it all work right: |
||||||
|
|
||||||
|
```go |
||||||
|
// Annotations is a set of key/value pairs representing a set of annotations. It |
||||||
|
// implements the Annotator interface along with other useful post-processing |
||||||
|
// methods. |
||||||
|
type Annotations map[interface{}]interface{} |
||||||
|
|
||||||
|
// Annotator is a type which can add annotation data to an existing set of |
||||||
|
// annotations. The Annotate method should be expected to be called in a |
||||||
|
// non-thread-safe manner. |
||||||
|
type Annotator interface { |
||||||
|
Annotate(Annotations) |
||||||
|
} |
||||||
|
|
||||||
|
// WithAnnotator takes in an Annotator and returns a Context which will produce |
||||||
|
// that Annotator's annotations when the Annotations function is called. The |
||||||
|
// Annotator will be not be evaluated until the first call to Annotations. |
||||||
|
func WithAnnotator(ctx context.Context, annotator Annotator) context.Context |
||||||
|
``` |
||||||
|
|
||||||
|
`Annotator` is designed like it is for two reasons. The more obvious design, |
||||||
|
where the method has no arguments and returns a map, would cause a memory |
||||||
|
allocation on every invocation, which could be a drag for long chains of |
||||||
|
contexts whose annotations are being evaluated frequently. The obvious design |
||||||
|
also leaves open questions about whether the returned map can be modified by |
||||||
|
whoever receives it. The design given here dodges these problems without any |
||||||
|
obvious drawbacks. |
||||||
|
|
||||||
|
The original implementation also had this unnecessary `Annotation` type: |
||||||
|
|
||||||
|
```go |
||||||
|
// Annotation describes the annotation of a key/value pair made on a Context via |
||||||
|
// the Annotate call. |
||||||
|
type Annotation struct { |
||||||
|
Key, Value interface{} |
||||||
|
} |
||||||
|
``` |
||||||
|
|
||||||
|
I don't know why this was ever needed, as an `Annotation` was never passed into |
||||||
|
nor returned from any function. It was part of the type `AnnotationSet`, but |
||||||
|
that could easily be refactored into a `map[interface{}]interface{}` instead. So |
||||||
|
I factored `Annotation` out completely. |
||||||
|
|
||||||
|
[component]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mcmp#Component |
||||||
|
[mctx]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mctx |
||||||
|
|
||||||
|
## mcfg, mrun |
||||||
|
|
||||||
|
The next package to tackle is [mcfg][mcfg], which deals with configuration via |
||||||
|
command line arguments and environment variables. The package is set up to use |
||||||
|
the old `mcmp.Component` type such that each component could declare its own |
||||||
|
configuration parameters in the global configuration. In this way the |
||||||
|
configuration would have a hierarchy of its own which matches the component |
||||||
|
tree. |
||||||
|
|
||||||
|
Given that I now think `mcmp.Component` isn't the right course of action it |
||||||
|
would be the natural step to take that aspect out of `mcfg`, leaving only a |
||||||
|
basic command-line and environment variable parser. There are many other basic |
||||||
|
parsers of this sort out there, including [one][flagconfig] or [two][lever] I |
||||||
|
wrote myself, and frankly I don't think the world needs another. So `mcfg` is |
||||||
|
going away. |
||||||
|
|
||||||
|
The [mrun][mrun] package is the corresponding package to `mcfg`; where `mcfg` |
||||||
|
dealt with configuration of components `mrun` deals with the initialization and |
||||||
|
shutdown of those same components. Like `mcfg`, `mrun` relies heavily on |
||||||
|
`mcmp.Component`, and doesn't really have any function with that type gone. So |
||||||
|
`mrun` is a gonner too. |
||||||
|
|
||||||
|
[mcfg]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mcfg |
||||||
|
[mrun]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mrun |
||||||
|
[flagconfig]: https://github.com/mediocregopher/flagconfig |
||||||
|
[lever]: https://github.com/mediocregopher/lever |
||||||
|
|
||||||
|
## mlog |
||||||
|
|
||||||
|
The [mlog][mlog] package is primarily concerned with, as you might guess, |
||||||
|
logging. While there are many useful logging packages out there none of them |
||||||
|
integrate with `mctx`'s annotations, so it is useful to have a custom logging |
||||||
|
package here. `mlog` also has the nice property of not being extremely coupled |
||||||
|
to `mcmp.Component` like other packages; it's only necessary to delete a handful |
||||||
|
of global functions which aren't a direct part of the `mlog.Logger` type in |
||||||
|
order to free the package from that burden. |
||||||
|
|
||||||
|
With that said, the `mlog.Logger` type could still use some work. It's primary |
||||||
|
pattern looks like this: |
||||||
|
|
||||||
|
```go |
||||||
|
// Message describes a message to be logged. |
||||||
|
type Message struct { |
||||||
|
Level |
||||||
|
Description string |
||||||
|
Contexts []context.Context |
||||||
|
} |
||||||
|
|
||||||
|
// Info logs an InfoLevel message. |
||||||
|
func (l *Logger) Info(descr string, ctxs ...context.Context) { |
||||||
|
l.Log(mkMsg(InfoLevel, descr, ctxs...)) |
||||||
|
} |
||||||
|
``` |
||||||
|
|
||||||
|
The idea was that if the user has multiple `Contexts` in hand, each one possibly |
||||||
|
having some relevant annotations, all of those `Context`s' annotations could be |
||||||
|
merged together for the log entry. |
||||||
|
|
||||||
|
Looking back it seems to me that the only thing `mlog` should care about is the |
||||||
|
annotations, and not _where_ those annotations came from. So the new pattern |
||||||
|
looks like this: |
||||||
|
|
||||||
|
```go |
||||||
|
// Message describes a message to be logged. |
||||||
|
type Message struct { |
||||||
|
Context context.Context |
||||||
|
Level |
||||||
|
Description string |
||||||
|
Annotators []Annotators |
||||||
|
} |
||||||
|
|
||||||
|
// Info logs a LevelInfo message. |
||||||
|
func (l *Logger) Info(ctx context.Context, descr string, annotators ...mctx.Annotator) |
||||||
|
``` |
||||||
|
|
||||||
|
The annotations on the given `Context` will be included, and then any further |
||||||
|
`Annotator`s can be added on. This will leave room for `merr` later. |
||||||
|
|
||||||
|
There's some other warts in `mlog.Logger` that should be dealt with as well, |
||||||
|
including some extraneous methods which were only used due to `mcmp.Component`, |
||||||
|
some poorly named types, a message handler which didn't properly clean itself |
||||||
|
up, and making `NewLogger` take in parameters with which it can be customized as |
||||||
|
needed (previously it only allowed for a single configuration). I've also |
||||||
|
extended `Message` to include a timestamp, a namespace field, and some other |
||||||
|
useful information. |
||||||
|
|
||||||
|
[mlog]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mlog |
||||||
|
|
||||||
|
## Future Work |
||||||
|
|
||||||
|
I've run out of time for today, but future work on this package includes: |
||||||
|
|
||||||
|
* Updating [merr][merr] with support for `mctx.Annotations`. |
||||||
|
* Auditing the [mnet][mnet], [mhttp][mhttp], and [mrpc][mrpc] packages to see if |
||||||
|
they contain anything worth keeping. |
||||||
|
* Probably deleting the [m][m] package entirely; I don't even really remember |
||||||
|
what it does. |
||||||
|
* Probably deleting the [mdb][mdb] package entirely; it only makes sense in the |
||||||
|
context of `mcmp.Component`. |
||||||
|
* Making a difficult decision about [mtest][mtest]; I put a lot of work into it, |
||||||
|
but is it really any better than [testify][testify]? |
||||||
|
|
||||||
|
[merr]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/merr |
||||||
|
[mnet]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mnet |
||||||
|
[mhttp]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mhttp |
||||||
|
[mrpc]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mrpc |
||||||
|
[m]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/m |
||||||
|
[mdb]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mdb |
||||||
|
[mtest]: https://pkg.go.dev/github.com/mediocregopher/mediocre-go-lib/mtest |
||||||
|
[testify]: https://github.com/stretchr/testify |
Loading…
Reference in new issue