Keep multiple traces

[nikaro]

Fix #11

[nikaro]

Should be good. At least it works for me.

If i run ansible-playbook -i inventory/local ignored/ping.yml ignored/ping2.yml, here is what i get :

trace/
├── 20211111134633-ignored-ping-yml.json
└── 20211111134633-ignored-ping2-yml.json

The only left issue that i can see is if someone, for example, run the same playbook at the exact same time (but for different hosts) on two different terminal panes. It can be easily solved by generating a random string next to the timestamp, but i don't want to complicate it too much. Let me know if it is worth it or not.

[mhansen]

Thanks for this contribution! I'm keen to have multiple timestamped traces. I'm less sure about deleting files. And I don't quite know how labelling files with the playbook name works (maybe it's fine, I'm not sure what a playbook is, or if there's always one, or if there's sometimes many). Mostly I want to trace an 'entire run' -- some logical program the user is waiting for. I'm not sure if splitting by playbook helps that or maybe it splits the trace into multiple parts?

[mhansen]

I think if we split this pull request into:

• adding a timestampped output file (with rfc3339 filename)
• removing the renaming
• updating the README.md to explain how to open the generated file

I could accept those noncontroversial changes, then maybe we could reconvene for the other changes (playbook names, rotation).

[nikaro]

Since i updated the PR GitHub does not let answer to the other in-code comments. So let me do it here.

this is a gnarly regex with a lot of backquotes :-)

just checking my understanding is this replacing . / and \ with -?

Yep, that's right. / (unix) and \ (windows) cause issue since they are directory separator. . is more a cosmetic change to avoid having trace file ending with .yml.json, but i don't mind ditching this one if it make it more difficult to read. I think we can do playbook._file_name.replace('/', '-').replace('\', '-'), this is less pythonic but may be simpler.

I wonder if we could avoid the need for this rename by defering opening the file until we get the v2_playbook_on_start callback.

Actually that's what i did in the last commit of this PR.

Just checking my understanding: does v2_playbook_on_start only get called once per execution of ansible? (I admit I get a big confused by some ansible concepts, like what is a play vs a playbook)

v2_playbook_on_start is called at each playbook start. For ansible-playbook myplaybook.yml it called once, but for ansible-playbook myplay1.yml myplay2.yml it is called twice. (i admit i discovered it by hacking with your plugin, that's the first time i play with callback code ^_^).

[nikaro]

And I don't quite know how labelling files with the playbook name works (maybe it's fine, I'm not sure what a playbook is, or if there's always one, or if there's sometimes many). Mostly I want to trace an 'entire run' -- some logical program the user is waiting for. I'm not sure if splitting by playbook helps that or maybe it splits the trace into multiple parts?

I think you are right, trace by "entire run" is better. I've not though/tested enough, but in a signle "playbook file" you can have multiple "plays" :

---
# playbooks/myapp.yml

- name: "DATABASES"
  hosts: "db_servers"
  roles:
    - "postgresql"

- name: "APPLICATION"
  hosts: "app_servers"
  roles:
    - "myapp"

For this single "playbook file", v2_playbook_on_start will be called twice, hence ending with two trace files...

I will update my PR.

[nikaro]

@mhansen it should be good know, let me know if there is something else and/or if i should squash my commits into one.

[mhansen]

Could you please squash these commits (not into one commit, but rather:) so that each commit is adding one atomic piece of functionality, and we aren't reverting features (like per-play naming) in later commits?

And could you remove the deletion of globbed files? It just feels too risky to me to merge something that deletes all the JSON files in a user's folder, even if it defaults to off. I could imagine deleting all the JSON in the user's home directory if they set that to be the output folder, for example.

[nikaro]

@mhansen it's done.

I also prefixed the output file with trace- (trace/trace-<timestamp>.json). Because if the script do not do the rotation itself, we still have to do it manually and you should feel safer this way: rm <output_dir>/trace-*.json :-)

[mhansen]

Thank you! LGTM. The atexit feels a little unfortunate that theres no callback, but I don't see an easy way around it?

Unless we make multiple traces per play, which I think I said not to before. Maybe I was wrong, maybe multiple traces per play would work better with the callbacks we have. Not sure. Let's merge for now and see how it runs.

[mhansen]

FYI I just released this as https://galaxy.ansible.com/mhansen/ansible_trace 0.0.5

On Wed, 17 Nov 2021 at 21:29, Nicolas Karolak @.***> wrote:

@mhansen https://github.com/mhansen it's done.

I also prefixed the output file with trace- (trace/trace-.json). Because if the script do not do the rotation itself, we still have to do it manually and you should feel safer this way: rm /trace-*.json :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mhansen/ansible-trace/pull/12#issuecomment-971442942, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZYOMX6CEKGWW6MR2A5LTUMN7Y3ANCNFSM5H2IJ4EA .