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

allow setting the logVerbosity of Documents created within the repo #118

Conversation

diederich
Copy link

@diederich diederich commented Jul 24, 2024

LogVerbosity on automerge's Document is internal and can only be specified on document creation.
This PR allows setting the default for this via the repository.

Note: My first approach was to add a case in LogComponent, though that also needs a logger. Given we don't really want to log into the Document's logger from the Repo, it felt better to add a separate public variable. Happy to adjust though!

@heckj
Copy link
Collaborator

heckj commented Jul 24, 2024

No qualms with where you're going - I haven't needed the logging for debugging (which is mostly where I use it) at the Document level outside of the repo, but totally game to enable it. What do you think about making another "component" for the logger, including in that private dictionary, and going from there? The public var makes it look more like you can "turn it on and off", where in practice its not nearly as configurable (enable at start or not), although I've been mostly using it only in tests (unit, functional, or interop/integration) when something was confusing to me - to debug.

Is part of the goal here to make it more dynamically configurable?

@diederich
Copy link
Author

No qualms with where you're going - I haven't needed the logging for debugging (which is mostly where I use it) at the Document level outside of the repo, but totally game to enable it.

We were debugging some timing issue and tried to crank up all logging to "eleven" to see what's going on. We only later found that compared to the Repo's use of the Logger infra, Doc's is quite minimal, so that didn't turn out to be helping in the end.

What do you think about making another "component" for the logger, including in that private dictionary, and going from there?

Yeah, I went that way initially but found adding a LogComponent also requires to return a Logger.
WDYT about adding it anyways and creating a Logger for the document here. The logger wouldn't currently be used though (which initially felt a bit weird, though it also doesn't really hurt, as I think they're initialised lazily)
We could also make the returned Logger from LogComponent optional, but that felt a bit much "burden" on the usage side just for being able to specify the Doc's logger.

The public var makes it look more like you can "turn it on and off", where in practice its not nearly as configurable (enable at start or not), although I've been mostly using it only in tests (unit, functional, or interop/integration) when something was confusing to me - to debug.

Yeah - agreed, it only works for documents loaded after setting.

Is part of the goal here to make it more dynamically configurable?

Nope, not at all. It was all just for local debugging.

With that all in mind, few ideas/options:

  • dropping the PR is fine, we didn't get as much help out of it as we thought. Having said that, others might fall into the same trap... :-)
  • I found the logging in the Repo (and its components) incredibly helpful for understanding more of the machinery. Extending the logging in Doc might be a good idea.
  • Doc's will likely always be created by the Repo in some way or another so passing the LogLevel in the initialiser seems the right choice.
  • We could pass the documentLogLevel in the initialiser(s) and default it to .errorOnly. No public API for changing and no additional Logger or Logger? required.

WDYT?

@heckj
Copy link
Collaborator

heckj commented Jul 25, 2024

The last option (passing a default logger of .errorOnly) matches the behavior of an Automerge document currently, but makes it explicit rather than implicit.

For the Document itself, there's little logging because there's little logic to log from - 99% if the logic there is all in the Rust library, which isn't as easily exposed to these sorts of mechanisms - and currently doesn't log. The places in Automerge-swift where I did lean into logging was in the Encoding and Decoding logic for the Codable support, so more specifically the AutomergeEncoder and AutomergeDecoder classes.

I personally found the deluge of logging data when I had everything enabled to be "too much" - I was swimming in log lines that obscured anything else I was looking for, which is why I went to the trouble to add the explicit logLevels and the hilariously wonky coding to keep the oslog() privacy mechanisms that are bound into the compiler. (I found it really, really annoying that I couldn't wrap or extend that, but with the compiler code generation bits, it just didn't fly)

`LogVerbosity` on automerge's Documents are internal and can only
be specified on document creation.
This PR allows setting the default for this via the repository.

Note: first approach was to add a case in `LogComponent`, though
that _also_ needs a logger. Given we don't really want to log into the
`Document`'s logger from the `Repo`, it felt better to add a separate
public variable.
while `DocumentStorage` has it's own logLevel, we didn't push this
into the Documents created from it. This adds another log level
and forwards it from the repo.
@diederich diederich force-pushed the feature/allow-specifying-document-logVerbosity branch from 920378d to 0d665c5 Compare July 29, 2024 19:55
@diederich
Copy link
Author

Rebase on current main and went with the init param route!

@diederich diederich closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants