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

Format trace to JSON format #754

Merged
merged 19 commits into from
Jan 21, 2025
Merged

Format trace to JSON format #754

merged 19 commits into from
Jan 21, 2025

Conversation

denismerigoux
Copy link
Contributor

Based on @rprimet's, excellent suggestion, we want to print the trace by the interpreter in a JSON format that could be exploitable by debug tools. However, the parsing of raw events to events is broken now and needs to be fixed. In the meantime, this PR prints the list of raw events on the standard output in JSON format.

@denismerigoux denismerigoux added ✨ enhancement New feature or request 🔧 compiler Issue concerns the compiler labels Dec 16, 2024
@denismerigoux denismerigoux requested a review from AltGr December 16, 2024 10:38
@rprimet
Copy link
Contributor

rprimet commented Dec 17, 2024

Note : it is possible that we are not outputting valid json yet (for instance, numbers are a footgun in json, and it seems that numbers with an unset fractional part are not accepted : e.g. 1.0 is valid but 1. isn't -- that should probably be an easy fix)

Copy link
Contributor

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

LGTM! An area that could be improved is the UI: we have 3 different flags for traces, and specifying an output file for example doesn't imply that the trace should be on.
Ideas:

  • make the trace output file be an optional argument of --trace, so that no extra flag is needed (note though: it could break existing scripts that do e.g. catala interpret -t foo.catala_en because foo.catala_en would be taken as the argument of -t ; we don't have that many scripts around and I still think it's ok, though)
  • add environment variables for the flags (in particular, that could be useful for the trace format)
  • infer the trace format from the output file name if .json

This may require some habit of Cmdliner so I can help ^^

runtimes/ocaml/runtime.ml Outdated Show resolved Hide resolved
@rprimet
Copy link
Contributor

rprimet commented Jan 17, 2025

Adding the output file as an option of the --trace argument appears to have broken some scripts ( https://github.com/CatalaLang/catala/actions/runs/12827450876/job/35769769947?pr=754 ) ; not sure what the best course of action is, reinstate a --trace-output flag or fix the invocation in the scripts using -- ?

@AltGr
Copy link
Contributor

AltGr commented Jan 17, 2025

yes that's the caveat I pointed to before: we should fix the scripts, either by expliciting -t - or by just moving the flag so that it doesn't appear right before the catala file name.
When we automatically handle the .json file extension, we should also add a warning pointing the correct course of action if we detect that the supposed trace file ends with .catala_?? (or even an error, to be totally certain that a catala file won't get overwritten!)

@rprimet
Copy link
Contributor

rprimet commented Jan 20, 2025

Now that the catala-example CI has a candidate fix ( CatalaLang/catala-examples#19 ), the main issue remaining in the CI is that some messages have a different format, due to a few issues of Message.log being replaced by straight Format.fprintf in the code of print_log (see for instance git diff HEAD..master compiler/shared_ast/interpreter.ml) ; as a result, there are a few line break inconsistencies and missing [LOG] prefixes. @AltGr any ideas on what may be the best fix, parametrize Message so that the ppf instance can be user-supplied or something else?

@AltGr
Copy link
Contributor

AltGr commented Jan 20, 2025

I wrote a workaround so that the format, when printing to the console, is the same as before (when in a file the [LOG] prefixes are removed, as they should be)

- don't duplicate Message.*ppf printers
- adjust some formatting details
@AltGr
Copy link
Contributor

AltGr commented Jan 21, 2025

@rprimet basically the last bug was that we were duplicating the formatter generated on top of stdout, which led to erratic behaviours when both versions were being used; should be all OK now.

@rprimet rprimet merged commit 8a6a4d1 into master Jan 21, 2025
5 checks passed
@rprimet rprimet deleted the trace_to_json branch January 21, 2025 15:24
@rprimet
Copy link
Contributor

rprimet commented Jan 21, 2025

Thanks @AltGr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 compiler Issue concerns the compiler ✨ enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants