janestreet / async_extra

Jane Street Capital's asynchronous execution library (extra)
MIT License
15 stars 8 forks source link

Workaround for removal of Log.of_lazy? #2

Closed seliopou closed 10 years ago

seliopou commented 10 years ago

The latest release of async_extra removed the Log.of_lazy function, which we use in the ocaml-openflow project. Is there a suggested way to work around the removal of this function? On first glance, it seems as though there is no way to reproduce its functionality, as the log level can be set on a Log.t, but it cannot be read. Any suggestion would be helpful. Thanks.

yminsky commented 10 years ago

Hmm. CC'ing Dave Powers, who I think removed the feature in question. I'm not sure why it's now gone, but you can get the same functionality back, using the sexp logger. In particular, here's the signature of the sexp function:

(* [sexp] logging of values without first converting them to a string. In the case where the log level would discard this message no string conversion will ever be done. ) val sexp : ?level:Level.t -> ?time:Time.t -> ?tags:(string * string) list -> t -> 'a -> ('a -> Sexp.t) -> unit

So if you wanted, you could call

Log.sexp log some_lazy_string (fun l -> Sexp.Atom (force l))

This is more awkward than the original lazy call, perhaps, but it should have the right semantics.

y

On Fri, Aug 1, 2014 at 2:41 PM, Spiros Eliopoulos notifications@github.com wrote:

The latest release of async_extra removed the Log.of_lazy function, which we use in the ocaml-openflow https://github.com/frenetic-lang/ocaml-openflow project. Is there a suggested way to work around the removal of this function? On first glance, it seems as though there is no way to reproduce its functionality, as the log level can be set on a Log.t, but it cannot be read. Any suggestion would be helpful. Thanks.

— Reply to this email directly or view it on GitHub https://github.com/janestreet/async_extra/issues/2.

seliopou commented 10 years ago

Thanks for the pointer, @yminsky. I tried it out and it didn't not produce the expected results. Though the comment on sexp indicates that it should have the semantics that I'm looking for, the implementation does not check the log level before applying the conversion function to the value. It therefore seems that lazy log generation is not possible using the new API alone.

Is there a chance that this functionality will return, or should we pursue our own workaround? Thanks.

Related to frenetic-lang/ocaml-openflow#148.

yminsky commented 10 years ago

I, at least, shepherded the removal of the function and can try to address the issue.

Laziness was removed as part of a slow series of internal changes meant to improve logging of structured values. During this we determined that laziness wasn't giving us the performance boost originally intended in the design, and was making some other internal examination difficult. So we removed it.

As you correctly indicate, the right workaround is to check the log level when logging. We realized after the initial change that this wasn't exposed, and have since minted and (internally) released a feature to fix this. That feature should make it to the external release soon.

The sexp function sadly won't let you work around this - at least in the current code, and the comment needs to be adjusted to reflect this.

In the previous code this behavior was a freebie from the underlying lazy. In the new code it's an accidental regression. At the moment I'm unsure whether we will just adjust the comment (making this a convenience function mostly) or whether we will make further internal changes to add back in some laziness. I suspect the former.

Feel free to reach out with any more questions/suggestions/use cases.

-David

On Fri, Aug 1, 2014 at 11:03 PM, Yaron Minsky yminsky@gmail.com wrote:

Hmm. CC'ing Dave Powers, who I think removed the feature in question. I'm not sure why it's now gone, but you can get the same functionality back, using the sexp logger. In particular, here's the signature of the sexp function:

(* [sexp] logging of values without first converting them to a string. In the case where the log level would discard this message no string conversion will ever be done. ) val sexp

: ?level:Level.t -> ?time:Time.t -> ?tags:(string * string) list -> t -> 'a -> ('a -> Sexp.t) -> unit

So if you wanted, you could call

Log.sexp log some_lazy_string (fun l -> Sexp.Atom (force l))

This is more awkward than the original lazy call, perhaps, but it should have the right semantics.

y

On Fri, Aug 1, 2014 at 2:41 PM, Spiros Eliopoulos < notifications@github.com> wrote:

The latest release of async_extra removed the Log.of_lazy function, which we use in the ocaml-openflow https://github.com/frenetic-lang/ocaml-openflow project. Is there a suggested way to work around the removal of this function? On first glance, it seems as though there is no way to reproduce its functionality, as the log level can be set on a Log.t, but it cannot be read. Any suggestion would be helpful. Thanks.

— Reply to this email directly or view it on GitHub https://github.com/janestreet/async_extra/issues/2.

seliopou commented 10 years ago

Thanks for the additional info. Our use case involved computing hashes and generating human-readable representations of packets when at the debug log level—something we'd like to avoid when at higher levels.

At the moment we already have a thin wrapper around the logging module that conforms to the old interface, so managing log levels may not be so difficult there. It would however be nice to be able to offload the management of that state to the underlying logging module. In order to do that, all that we'd need is a function to read the current log level, and it sounds like you've already implemented internally. If that's the case, do you have any idea when that'll make it into a release? If it's going to be soon, then we can just wait for that to come through before making any changes.

yminsky commented 10 years ago

Yes. This is already in the latest release internally, and that should go out in the next release, which is a week or two away.

dpowers commented 10 years ago

I did another review of the code over the weekend and I think we can get back (mostly) to the world we had before the recent changes landed.

In the short term, you should have (val level : t -> Level.t) in the next release.

In the release after that I hope to restore the full laziness of sexp. There will still be a slight advantage in speed to using the if test on level in your client code because some internal processing (especially in the printf-like functions where I don't have a good way to avoid the string creation) can't be avoided, but sexp at least should be very efficient.

seliopou commented 10 years ago

Putting this to use in frenetic-lang/ocaml-openflow#171. Thanks!