Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

CP-45795: Decompress compressed trace files without Forkexecd #6293

Merged
merged 1 commit into from
Feb 20, 2025
Merged

CP-45795: Decompress compressed trace files without Forkexecd #6293

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions ocaml/xs-trace/xs_trace.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module Exporter = struct
if json <> "" then
match Tracing_export.Destination.Http.export ~url json with
| Error err ->
Printf.eprintf "Error: %s" (Printexc.to_string err) ;
Printf.eprintf "Error: %s\n" (Printexc.to_string err) ;
exit 1
| _ ->
()
Expand All @@ -34,18 +34,17 @@ module Exporter = struct
(* Recursively export trace files. *)
Sys.readdir path
|> Array.iter (fun f -> Filename.concat path f |> export_file)
| path when Filename.check_suffix path ".zst" ->
(* Decompress compressed trace file and decide whether to
treat it as line-delimited or not. *)
let ( let@ ) = ( @@ ) in
let@ compressed = Unixext.with_file path [O_RDONLY] 0o000 in
let@ decompressed = Zstd.Fast.decompress_passive compressed in
if Filename.check_suffix path ".ndjson.zst" then
let ic = Unix.in_channel_of_descr decompressed in
Unixext.lines_iter submit_json ic
else
let json = Unixext.string_of_fd decompressed in
submit_json json
| path when Filename.check_suffix path ".zst" -> (
(* Decompress compressed trace file and submit each line iteratively *)
let args = [|"zstdcat"; path|] in
let ic = Unix.open_process_args_in args.(0) args in
Unixext.lines_iter submit_json ic ;
match Unix.close_process_in ic with
| Unix.WEXITED 0 ->
()
| _ ->
Printf.eprintf "File %s exited with non-zero\n" path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use open_process_args_in as this is currently a string that is interpreted by the shell and that always has the risks of funny problems with spaces in names and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the difference between the two. I thought I should just be able to change it to:
let ic = Unix.open_process_args_in "zstdcat" [|path|] in
but it gives the error stdin is a console, aborting when the original code does not have this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even the documentation says Unix.open_process_args_in is a safe replacement. I assume that stdin is handled differently by the shell (underlying open_proess and zstdcat. What stdin does the error even refer to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is ic closed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to satisfy the convention that argv[0] is the program's name/path when using Unix.open_process_args_in. The usual convention is:

let args = [|"zstdcat"; ...|] in
let ic = Unix.open_process_args_in args.(0) args in
...

The error you're getting is the same as if you just executed zstdcat with no user-supplied arguments from a shell:

~ zstdcat
stdin is a console, aborting

Copy link
Contributor Author

@snwoods snwoods Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is ic closed?

It's closed on line 41 match Unix.close_process_in ic with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to satisfy the convention that argv[0] is the program's name/path

Ahhh of course. I even read that statement but I think I glossed over it because I'm obviously passing the process name already, but it needs to be in args too! So it's assuming my first arg (path) is the name and therefore I haven't provided any filenames...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change is now understood; can we change this and merge then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I wanted to test the change first and Jaeger was experiencing unrelated flakiness. Tried again now and it's all good 👍

)
| path when Filename.check_suffix path ".ndjson" ->
(* Submit traces line by line. *)
Unixext.readfile_line submit_json path
Expand Down
Loading