Skip to content

Adds a job info page for scheduled jobs which doesn't truncate args#1076

Merged
mperham merged 7 commits intosidekiq:masterfrom
jonhyman:feature/arg-scroll
Jul 28, 2013
Merged

Adds a job info page for scheduled jobs which doesn't truncate args#1076
mperham merged 7 commits intosidekiq:masterfrom
jonhyman:feature/arg-scroll

Conversation

@jonhyman
Copy link
Contributor

Given the ease of this change, consider this the issue per Contributing.md.

The truncation of arguments at 100 characters in sidekiq-web makes it basically unusable for the scheduled jobs for me since our job classes are namespaced into modules and Moped::BSON::ObjectIds serialize themselves starting with !ruby/object:Moped::BSON::ObjectId which is more than 1/3 of the allotted space anyway.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 6ce788b on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@mperham
Copy link
Collaborator

mperham commented Jul 25, 2013

Screenshot please.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 6ce788b on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@jonhyman
Copy link
Contributor Author

I'm in Chrome. I scrolled down a bit then stopped so you can tell:

screen shot 2013-07-25 at 6 45 23 pm

When scrolling, that field looks like this
screen shot 2013-07-25 at 6 45 34 pm

@jonhyman
Copy link
Contributor Author

It looks similar in the other two places.

@jonhyman
Copy link
Contributor Author

With multiple jobs, scrolling each:

screen shot 2013-07-25 at 6 50 20 pm

@mperham
Copy link
Collaborator

mperham commented Jul 25, 2013

I still think there needs to be a cutoff character count, just to keep the page from being 5MB if someone has large payloads. Maybe 2000?

@jonhyman
Copy link
Contributor Author

Yeah, I guess someone could have a huge payload. The page limits to 25, so the limit on args could be pretty high. Even at 20,000 characters, that's just shy of 500kb for the args, which I think is still acceptable. Thoughts on that?

@mperham
Copy link
Collaborator

mperham commented Jul 25, 2013

No, they can still click thru to the individual job page if they want to see the entire thing. 2k is more than reasonable for an overview.

@jonhyman
Copy link
Contributor Author

Ah, that could explain one of my problems. You can't click through to any jobs page for Scheduled Jobs.

@mperham
Copy link
Collaborator

mperham commented Jul 25, 2013

Sounds like your next PR then. :-)

@jonhyman
Copy link
Contributor Author

Yep!

@jonhyman
Copy link
Contributor Author

Though, is there a jobs page at all for scheduled jobs? I just shut down some workers to let jobs queue up and look at mysite.com/sidekiq/queues/default and there's a "Show All" button to expand the args, but not an individual page for an enqueued item.

@mperham
Copy link
Collaborator

mperham commented Jul 25, 2013

I believe Retries allow you to click thru - you would just do the same thing since Retries are just scheduled jobs.

@jonhyman
Copy link
Contributor Author

Okay, good to know it exists. I'll poke around tomorrow/this weekend probably.

…which removed arg truncation from overview pages.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 5e9bfa7 on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@jonhyman
Copy link
Contributor Author

@mperham I've updated this to add a status page for scheduled jobs. I also upped the truncation to 2000 characters on the summary pages and display the full argument set on the status page.

@mperham
Copy link
Collaborator

mperham commented Jul 27, 2013

Great progress, you're missing tests for the new endpoints.

Could you remove all your changes around data population in contributing.md and config.ru? I'd prefer to keep this PR focused on the functional change.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 963e4e9 on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 963e4e9 on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@jonhyman
Copy link
Contributor Author

Updated with tests. The config.ru file already had some commented out code which populates parts of the view before I made this PR; I did not remove it, I just reverted my changes.

test/test_web.rb Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this JID to 'shouldntexist' so its clear to the reader you're testing the 404 behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameters to look up a scheduled job should include the score so the lookup is O(logN) in Redis. Otherwise it's an O(N) operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the jid per your comment and added a score, too, so it's more clear.

The parameters to look up a scheduled job do include the score; in get "/scheduled/:key the line of code to look up a job is:

@job = Sidekiq::ScheduledSet.new.fetch(*parse_params(params['key'])).first

and in scheduled.slim, the link is generated using:

a href="#{root_path}scheduled/#{job_params(msg, score)}"== relative_time(Time.at(score))

Let me know if I'm missing something here.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 820657a on jonhyman:feature/arg-scroll into 0ae5ba4 on mperham:master.

@mperham mperham merged commit 820657a into sidekiq:master Jul 28, 2013
@mperham
Copy link
Collaborator

mperham commented Jul 28, 2013

Nice work!

@jonhyman jonhyman deleted the feature/arg-scroll branch July 29, 2013 19:03
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.

3 participants