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

flux-job: Add --time-format option #2142

Merged
merged 5 commits into from Apr 29, 2019

Conversation

@chu11
Copy link
Contributor

commented Apr 25, 2019

As discussed in #2074, it'd be nice to format job eventlog entry timestamps.

  • Support a new --time-format option in flux-job that takes raw, iso, and offset as inputs. Outputting floating point value, ISO8601, or offset from 0.0 (for the first entry).

  • I considered making flux-job eventlog and flux-kvs eventlog get consistent with each other (both have --format and --time-format options), but my gut told me that flux kvs eventlog get doesn't have to support all of the "pretty" options that flux-job does. The flux kvs option is more for debugging/testing?

So I elected to have flux kvs eventlog get just output timestamps in floating point and that's it.

Obviously we can do differently, dunno what people's opinions are.

@chu11 chu11 force-pushed the chu11:issue2074 branch from c357f7d to d1f3371 Apr 26, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Just a thought, but should we make the time offset format the default? It seems the most useful.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

That does make sense for eventlog. But should it be default for wait-event? The one event normally output by wait-event has no context for an offset time?

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

The one event normally output by wait-event has no context for an offset time?

flux_job_event_watch() always returns the first event (correct?), whether we display it or not, so I guess we could still display it as an offset from the submit event. Whether that's the right default for wait-event or not is another question. I would lean towards yes. I take it you were leaning the other way?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

flux_job_event_watch() always returns the first event (correct?), whether we display it or not, so I guess we could still display it as an offset from the submit event.

Yes, with --time-format=offset, wait-event will output the correct offset regardless if the submit event is output.

Whether that's the right default for wait-event or not, I would lean towards yes. I take it you were leaning the other way?

My feeling wasn't strong, but just wondering if the offset wouldn't make sense given lack of context.

> flux job eventlog 139821318144
0.000000 submit userid=8556 priority=16 flags=0
0.013542 depend
0.016902 alloc note="rank[0-3]/core0"

makes sense to me b/c you see submit at 0 and it increasing. But with only one event:

>flux job wait-event 139821318144 alloc
0.016902 alloc note="rank[0-3]/core0"

vs

>flux job wait-event 139821318144 alloc
2019-04-29T17:08:08.304609Z alloc note="rank[0-3]/core0"

vs

>flux job wait-event 139821318144 alloc
1556557688.304610 alloc note="rank[0-3]/core0"

the iso output seems to make the most sense to me by default?

But if eventlog defaults to offset, we should probably default to offset as well for consistency.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I see your points and don't have a really strong feeling either way. @grondo want to cast a vote here?

@grondo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

I'm fine with eventlog and wait-event both defaulting to raw timestamp, though I don't have a strong opinion. If the default behavior does change, the sched-bench.sh script will have to updated I think.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Works for me.

chu11 added 5 commits Apr 24, 2019
Do not output eventlog entry timestamps in ISO 8601 format, just
output them as raw floating point values.
Add a --time-format option to eventlog and wait-event commands, allowing
user to output time in raw floating point or ISO 8601 format.

Add unit tests.

Fixes #2074
Support --time-format=offset option, which will output the offset time
each eventlog entry was written, assuming the first eventlog entry was
at time 0.0.  This is a useful feature to see how long stages/events
took within a job.

Add unit tests.
Create struct event_format to store common event entry formatting
information.  Create common function entry_format_parse_options() to
parse formatting options.
@chu11 chu11 force-pushed the chu11:issue2074 branch from d1f3371 to b0f8f20 Apr 29, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

ok, so I guess we're keeping raw as the default. Rebased and re-pushed.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 29, 2019

Codecov Report

Merging #2142 into master will increase coverage by 0.02%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #2142      +/-   ##
==========================================
+ Coverage   80.39%   80.42%   +0.02%     
==========================================
  Files         200      200              
  Lines       31733    31745      +12     
==========================================
+ Hits        25513    25530      +17     
+ Misses       6220     6215       -5
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 81.81% <100%> (-0.12%) ⬇️
src/cmd/flux-job.c 86.6% <95.12%> (+0.24%) ⬆️
src/common/libflux/mrpc.c 87.74% <0%> (-1.19%) ⬇️
src/common/libflux/message.c 81.64% <0%> (+0.36%) ⬆️
src/broker/modservice.c 79.8% <0%> (+0.96%) ⬆️
src/common/libflux/response.c 80.5% <0%> (+1.25%) ⬆️
src/modules/barrier/barrier.c 80.53% <0%> (+2.01%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Thanks!

@garlick garlick merged commit 98dba27 into flux-framework:master Apr 29, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 95.23% of diff hit (target 80.39%)
Details
codecov/project 80.42% (+0.02%) compared to 6475001
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.