weissi / swift-server-logging-api-proposal

SSWG Proposal for Logging API
9 stars 8 forks source link

remove Metadata.Value.lazy #18

Closed weissi closed 5 years ago

weissi commented 5 years ago

sorry @ktoso , does this make sense?

Motivation:

Two reasons for the removal:

  1. I don't see a compelling reason to add lazy metadata to the metadata storage. Lazy metadata makes sense if it's passed directly to a log method like

    logger.info("foo", metadata: somethingExpensive())

    but I don't think this makes a whole lot of sense

    logger[metadataKey: "foo"] = .lazy({ somethingExpensive() })

    why? It's very likely that it'll get evalutated because any next log message that gets logged will force it but it's totally unclear which one that'll be.

  2. More importantly, the lazy metadata can't be implemented correctly because we can't memoize the result of the expensive computation because we can only mutate the LogHandler in mutating methods and the log methods aren't mutating.

ktoso commented 5 years ago

Very tricky! We've talked about this with @weissi some more and I've dug into it some more.

Sadly, I think Johannes is right, that while the lazy itself "works", it's a "fake works"... (Sad not because he's right, sad because it can't work.) We are unable to satisfy our ability to allow Loggers being structs + lets AND make the lazy perform it's desired utility since:

Thought experiment:

Summary: We can't provide storage in struct style LogHandlers (without hackery) for caching a rendered metadata. Thus the .lazy may not yield as much benefit as we hoped, since it would be not "render at most once" but actually "render zero times... or n times" which is even worse (!).

We can't drop allowing loggers to be structs. Thus, we can't provide the storage, thus the lazy loses its utility.

"Bummer!" as they say; Therefore I'm 👍 on removing the .lazy (which I had initially fought for 😜). I'm very glad we had this discussion though, and we were able to make the per parameter metadata () -> which is a good step here, and this can be used to avoid rendering on an one off basis.

I will explore patterns we can recommend if such rendering avoidance will be sought after though, I'm sure there is a way, though it needs not impact the API with a lazy like here proposed.

weissi commented 5 years ago

thank you @ktoso