Fixes #33223 - Improving Job Invocation Report Template #8711
Conversation
|
Issues: #33223 |
ares
left a comment
ares
left a comment
There was a problem hiding this comment.
Since you'll be modifying this report, could you also please add report_headers definition? Ideally just after metadata, like other reports do. The purpose is to list all headers as strings so that if the load_hosts doesn't iterate over anything (e.g. search syntax does not find any host) we still print at least the headers.
| ) -%> | ||
| <%- outputs[output['output_type']] << output['output'] -%> | ||
| <%- end -%> | ||
| <%- report_row(host: host.name, stdout: outputs['stdout'], stderr: outputs['stderr'], debug: outputs['debug']) -%> |
There was a problem hiding this comment.
It would be also good to keep the time information. I'd call it "finished at" and we should put the time when the job finished for a given host.
app/views/unattended/report_templates/jobs_-_invocation_report_template.erb
Show resolved
Hide resolved
| <%- load_hosts(search: search).each do |batch| -%> | ||
| <%- batch.each do |host| -%> | ||
| <%- task = invocation.sub_task_for_host(host) -%> | ||
| <%- outputs = { 'stdout' => [], 'stderr' => [], 'debug' => [] } -%> |
There was a problem hiding this comment.
Not sure if there's anything we can do about it, but I think this might significantly increase the memory usage when generating a report for invocations that had very long outputs.
There was a problem hiding this comment.
It was a long array of short strings, now it's a smaller array of longer strings. The amount of characters though remains the same. The outputs hash is cleared for every job and is not linked from anywhere, so GC should deallocate that. Or did you mean some other part?
There was a problem hiding this comment.
i don't recall the internals of report_row, don't we stream/write it one row at a time? so instead of loading one output line every iteration and writing it, we load all the outputs to memory before writing them all in one go.
There was a problem hiding this comment.
We don't stream that, we're storing this in DB and allow to download that rendered report later. So we load everything into memory, then store it to DB. When it's downloaded, we load the whole thing from PG to Rails process and send the file. Streaming was never finished #8136
| 'stderr': outputs['stderr'], | ||
| 'debug': outputs['debug'], | ||
| 'Result': task.result, | ||
| 'Finished': task.ended_at, |
There was a problem hiding this comment.
the previous version had format_time, don't we need that anymore?
|
I've updated PR and tests are green. I think it's ready to merge. |
ares
left a comment
ares
left a comment
There was a problem hiding this comment.
Still one thing to be addressed, the actual output. I guess we should do something like
outputs['stdout'].join("\n")
This will break YAML and JSON and will not work well with HTML.
Out of the scope but a great follow up - introduce a report_line_break macro that would based on the report_format return either "\n" or "
" that we could use in this template (for better formatting of html output). For JSON or YAML we may need "\n" to not break the format.
Another out of the scope but a good follow up, during the testing I noticed the format_time macro is actually broken as it shows the date as 2021-8-6 while I guess it should be 2021-08-06.
| <%- end -%> | ||
| <%- report_row( | ||
| 'Host': host.name, | ||
| 'stdout': outputs['stdout'], |
There was a problem hiding this comment.
this is now array of strings, should we join them into a single string? the result otherwise contains
[
"first line",
"second line",
...
in case of CSV the square brackets and quotes are removed, but every line is delimitered with comma. The same applies to stderr and potentially debug.
There was a problem hiding this comment.
Thanks for the comment! I updated the PR suggested way but little differently. I was struggling with concatenation of the future HTML output so after discuss with @ezr-ondrej I take another approach. Instead of having macro that only print correct line break for right output I made the macro that takes array of values and returns string with values that are chained with right line break.
theforeman-bot
added
Waiting on contributor
Needs re-review
and removed
Needs re-review
Waiting on contributor
labels
| when 'application/json', 'text/yaml' | ||
| array.join("\\n") | ||
| when 'text/html' | ||
| array.map(&:dump).join("<br>").html_safe |
There was a problem hiding this comment.
Perhaps this could be
| array.map(&:dump).join("<br>").html_safe | |
| array.map! {|str| CGI.escapeHTML(str)} | |
| array.join("<br>").html_safe |
to ensure the content is truly safe before marking it as such?
Also, using .map! will avoid creating an extra intermediate array which could be large when handling command outputs.
|
|
||
| apipie :method, "Return the concatenated array with correct line break" do | ||
| desc 'This method returns concatenated array with correct line break with the respect of output format. | ||
| For HTML output, it joins array members with <br> tag otherwise it uses a /n character. |
There was a problem hiding this comment.
typo:
| For HTML output, it joins array members with <br> tag otherwise it uses a /n character. | |
| For HTML output, it joins array members with <br> tag otherwise it uses a \n character. |
ares
left a comment
ares
left a comment
There was a problem hiding this comment.
When I tested this I got a familiar error
Safemode doesn't allow to access 'join_with_line_break' on #<Safemode::ScopeObject>
I suppose we need to add this to method to the allow list in order for it to work.
Updating the template to merge outputs lines to one batch to show them at one line.
|
I've updated PR and I think it's ready to merge. |
ezr-ondrej
added
the
Needs cherrypick
ezr-ondrej
removed
the
Needs cherrypick
5 participants
Updating the template to merge outputs lines to one batch to show them at one line.
Previous version:
It works perfectly, however the resulting report can be really hard to read for human because actual output for one host is at many lines and which one is a bad one? The image shows some Ansible command only for one host and yes, it's shows everything you need to know but user experience is that great. So I made this update:
There is a simplification to only four columns - the name of host and types of outputs. Every type of output is merged with respective types. I'm aware of destroying chronological order of outputs from a job but I think it's worth it because user can just look at the "stderr" column and see what can go wrong and he don't have to dig into several logs from previous version.