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

Annotate all runner_on_* events with their start/stop/duration times #405

Merged
merged 1 commit into from
Mar 16, 2020
Merged

Annotate all runner_on_* events with their start/stop/duration times #405

merged 1 commit into from
Mar 16, 2020

Conversation

wenottingham
Copy link
Contributor

This is incredibly useful information to have for a variety of needs.

Stolen from https://github.com/ansible/ansible-baseline/.

@ansible-zuul
Copy link
Contributor

ansible-zuul bot commented Jan 21, 2020

Build failed.

This is incredibly useful information to have for a variety of needs.
@ansible-zuul
Copy link
Contributor

ansible-zuul bot commented Jan 21, 2020

Build succeeded.

@AlanCoding
Copy link
Member

Right now I also have #404 open.

Some quick thoughts:

  • if a client wants the duration, they can subtract the values on their own, no need to do it here
  • the start timestamp is duplicated with the created timestamp, ideally we should only keep one
  • this doesn't work for verbose events, and I don't know if it can be made to work

@wenottingham
Copy link
Contributor Author

if a client wants the duration, they can subtract the values on their own, no need to do it here

I disagree - we want to avoid as much post-processing of the returned events as possible - if you're operating across one million events doing data processing, having to query all the events and then compute can be expensive.

the start timestamp is duplicated with the created timestamp, ideally we should only keep one

Will look.

@AlanCoding
Copy link
Member

This is the other place this is done:

event_dict['created'] = event_data.get('created', datetime.datetime.utcnow().isoformat())

That also does isoformat

@wenottingham
Copy link
Contributor Author

This is the other place this is done:

event_dict['created'] = event_data.get('created', datetime.datetime.utcnow().isoformat())

That also does isoformat

That's when it processes it from the disk, if I'm reading that right, which is already after the event has finished. That's sufficiently removed from the actual event generation in Ansible that I'd prefer to do that directly on the task start.

@AlanCoding
Copy link
Member

That's when it processes it from the disk, if I'm reading that right, which is already after the event has finished.

I do see now how these are really different animals. Logic for created is in generation of the begin dict, but I think that also applies to all the timestamps saved in this PR too. Aside from time spent in the callback plugin itself, the start time should be the same as the created timestamp for the runner_on_start events and the end time should be the same as the created for the runner_on_ok/skipped/etc. events. So I do think that all of this should be reconstructable from the client's perspective.

Copy link
Member

@matburt matburt left a comment

Choose a reason for hiding this comment

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

This is probably the best place to put this information. I think I'm good with this.

@AlanCoding
Copy link
Member

⚠️ D-D-D-DANGER ZONE ⚠️

This PR is targeting the master branch, but the default branch was changed to devel. If you can change your base branch, please do that now. Otherwise you may need to close and make a new PR.

@AlanCoding AlanCoding changed the base branch from master to devel February 17, 2020 16:22
@AlanCoding
Copy link
Member

Apparently it let me, so I went ahead and changed the base

@@ -498,6 +520,7 @@ def v2_runner_on_start(self, host, task):
host=host.get_name(),
task=task
)
self._host_start[host.get_name()] = current_time()
Copy link
Contributor

Choose a reason for hiding this comment

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

@matburt @wenottingham,

This doesn't work on Ansible pre-2.8, because runner_on_start was introduced in 2.8:

ansible/ansible#47684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants