-
Notifications
You must be signed in to change notification settings - Fork 13
Use trace instead of tracing #57
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine! The backend wasn't really meant to be used directly but it's fine, and you made sense of the API just fine :). I think you could just use Trace_subscriber.Subscriber.Callbacks
callbacks but it's really a detail.
if evt.kind = SpanBegin then Trace.Event.Duration_begin.encode | ||
else Trace.Event.Duration_end.encode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could direct use the Trace_fuchsia.Subscriber' callbacks here, too, without going through
Trace.Event`.
| Counter value -> | ||
Trace.write_counter trace.file ~thread ~category ~name ~time | ||
~args:[ ("v", Int value) ] | ||
Trace.Event.Counter.encode trace.buf ~t_ref ~name ~time_ns ~args:[ ("v", A_int value) ] () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, with subscriber
Thanks @c-cube ! I had a quick look and I think I might keep it as is just to keep it consistent in which module we use to make the events? I had a look for something for an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @patricoferris. I'm approving this based on @c-cube's review.
Yes, it's fine to rely on it :)
|
Let me know when it is ready to merge. |
f9b4f66
to
3f5f492
Compare
I think this should be good to go now @kayceesrk -- the codebase could use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @patricoferris minimising the dependencies makes sense.
Please remove ocaml_intrinsics
then it looks good to go.
I'm getting an exception using this @patricoferris @c-cube $ OCAMLRUNPARAM=b,s=8192k,o=40 olly trace infer-multicore-5-3.8192k.nopoll.trace "../infer/infer/bin/infer analyze --no-progress-bar --no-report --multicore -j 16"
Found 747 source files to analyze in /home/tsmc/openssl-1.0.2d/infer-out
olly: internal error, uncaught exception:
Failure("fuchsia: buffer is too small (available: 24 bytes, needed: 32 bytes)")
Raised by primitive operation at Runtime_events in file "runtime_events.ml" (inlined), lines 409-410, characters 0-76
Called from Olly_common__Launch.collect_events in file "lib/olly_common/launch.ml", line 105, characters 4-56
Called from Stdlib__Fun.protect in file "fun.ml", line 34, characters 8-15
Re-raised at Stdlib__Fun.protect in file "fun.ml", line 39, characters 6-52
Called from Stdlib__Fun.protect in file "fun.ml", line 34, characters 8-15
Re-raised at Stdlib__Fun.protect in file "fun.ml", line 39, characters 6-52
Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 24, characters 19-24
Called from Cmdliner_eval.run_parser in file "cmdliner_eval.ml", line 35, characters 37-44
|
There seems to be a buffer size check error in trace-fuchsia. Would you mind pinning edit: is there a chance that multiple threads might be accessing the fuchsia exporter at once? With |
CHANGES: * Use trace instead of tracing (tarides/runtime_events_tools#57 tarides/runtime_events_tools#59, @patricoferris @tmcgilchrist) * Add an option to control sleep interval between calls to read_poll (tarides/runtime_events_tools#60, @tomjridge @tmcgilchrist)
CHANGES: * Use trace instead of tracing (tarides/runtime_events_tools#57 tarides/runtime_events_tools#59, @patricoferris @tmcgilchrist) * Add an option to control sleep interval between calls to read_poll (tarides/runtime_events_tools#60, @tomjridge @tmcgilchrist)
This switches the fuchsia format backend away from using
tracing
totrace
.I find myself frequently reaching for these tools when doing some light performance analysis. Unfortunately, the dependency cone is quite large because of
tracing
pulling in lots of Janestreet packages.trace
has a much smaller dependency cone. The transitive dependencies have gone from 129 to 51 (as reported byopam list --required-by=runtime_events_tools -s | wc -l
)!The downside is that the fuchsia backend to
trace
is a little confusing to use (as it is intended to trace your own program manually via theTrace
module, not to be used directly to output a fuchsia trace). Maybe @c-cube can make sure I am holding it correctly :) (no worries if not!) ?