Updated documentation, esp. conclusion.md, and fixed add_handler, STDOUT, current_module() #137
Conversation
Replaced using Requests with using HTTP Properly handle unfinished @async log calls Replaced function log with function emit Replaced myid with getpid
Replaced `STDOUT` with `stdout`. Replaced `current_module()` with `@__MODULE__`. Replaced `myid()` with `getpid()`.
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
- Coverage 96.06% 95.95% -0.11%
==========================================
Files 12 12
Lines 305 297 -8
==========================================
- Hits 293 285 -8
Misses 12 12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR? Sorry it's taken a while to get to it, but I've made a few recommendations to help get this merged. It might also be worth breaking up the conclusion example updates from the rest of the 1.x docs updates, as the latter will likely to be merged more quickly.
| # A flush for @async tasks, to guarantee that the logs will have some time to finish writing, hooked in with atexit lazily | ||
| function finish_rest_log_tasks(timeout=5.0) | ||
| timer = Timer(timeout) | ||
| while any(t -> !istaskdone(t), REST_LOG_TASKS) && isopen(timer) |
There was a problem hiding this comment.
Couldn't you just use timedwait for this?
There was a problem hiding this comment.
I haven't used timedwait before. If it achieves the same functionality, sure!
| end | ||
|
|
||
| # A flush for @async tasks, to guarantee that the logs will have some time to finish writing, hooked in with atexit lazily | ||
| function finish_rest_log_tasks(timeout=5.0) |
There was a problem hiding this comment.
Since this is only ever used once couldn't you just define the logic here inside a do-block at the Base.atexit call?
There was a problem hiding this comment.
Also, why make timeout an argument if we never change it?
There was a problem hiding this comment.
The examples for hooking into the atexit that I've seen before require registering a zero argument function. I don't know if a do block would work. Do you have an example of doing that?
As for the timeout, I think there is a chance someone would want to manually call this function. At least I did while I was testing it. For my splunk server it needed at least 2 seconds to finish sending a log REST message if I logged and closed julia right away.
There was a problem hiding this comment.
The examples for hooking into the atexit that I've seen before require registering a zero argument function. I don't know if a do block would work. Do you have an example of doing that?
julia> Base.atexit() do
println("Goodbye!")
end
julia> exit()
Goodbye!
As for the timeout, I think there is a chance someone would want to manually call this function. At least I did while I was testing it. For my splunk server it needed at least 2 seconds to finish sending a log REST message if I logged and closed julia right away.
That's fair. Maybe the timeout should be a property of the type? Figuring out the best timeout value would then be configurable as part of the existing public API.
| function println(io::REST, level::AbstractString, msg::AbstractString) | ||
| uri = "https://$(io.account_uri)/$(io.app_name)/$level?AccessKey=$(io.access_key)" | ||
| @async put(uri; data=msg) | ||
| global REST_LOG_TASKS = [] |
There was a problem hiding this comment.
Might be better to store this as a Channel in the REST type to avoid having globals floating around? We could then control the size of the queue and have log writing wait until other requests have completed. The cleanup in the Base.atexit could then be done in a finalizer.
There was a problem hiding this comment.
Also, this is unrelated to your PR, but I wonder if this example would be simpler if we just made the REST a Handler rather than an IO. This seems complicated enough that it might warrant it now.
There was a problem hiding this comment.
A channel in the struct is probably better, especially if it doesn't touch the global scope. I would assume a channel could take task handles properly.
My logging to Restful interfaces is pretty cookie cutter based on this new code. Adding it as an included Handler would be nice. Though I don't need CSV, just json.
After I finished this PR, I also found I needed to filter out the stack trace more than what Memento was doing, so I added some more lines to remove the stack trace origination out of this new file's calls to get the trace.
There was a problem hiding this comment.
A channel in the struct is probably better, especially if it doesn't touch the global scope. I would assume a channel could take task handles properly.
Yeah, it's just a reference. I think they channel solution would look something like this:
julia> using Distributed
# Construction of the task channel
julia> taskref = Ref{Task}();
julia> c = Channel{Task}(10; taskref=taskref) do ch
while isopen(ch) || isready(ch)
t = take!(ch)
resp = timedwait(() -> istaskdone(t), timeout)
# resp can be :ok, :timed_out or :error
end
end
Channel{Task}(sz_max:10,sz_curr:0)
julia> julia> put!(c, @async 2 + 4)
Task (done) @0x0000000109031450
# Registering cleanup on object cleanup
# Near end of handler constructor
finalizer(handler) do
timedwait(() -> istaskdone(handler.taskref), timeout)
end
After I finished this PR, I also found I needed to filter out the stack trace more than what Memento was doing, so I added some more lines to remove the stack trace origination out of this new file's calls to get the trace.
Would you mind posting that logic in a separate MR? That sounds like a bug we should fix.
|
Alright, the non-example commits seem pretty simple, so I'm going to cherry-pick them into a separate PR that we can merge quickly. |
Mostly documentation fixes.
Conclusion.md is much more functional than it was before.
In my application logs are often sent or written right as julia exits, and the
@asynclogs wouldn't work in that case, so I added anatexit()hook for cleaning up the REST log tasks in the conclusion.md example to handle this case.