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

Refactor bug reporting into plugins / Remove bug report mako #4305

Merged
merged 33 commits into from Aug 10, 2017

Conversation

Projects
None yet
5 participants
@erasche
Copy link
Member

commented Jul 12, 2017

I've extended bug reports to use a more plugin style infrastructure. Instead of "user submitted = email, tool error = sentry (if configured)", the admin can decide how they want to handle bug reports. Do they want to submit sentry reports only when the user clicks the bug report? Do they want to send emails on every single failed tool? Do they have some third party system they wish to integrate with?

I've refactored Email + Sentry into two plugins, and provided a 'JSON' output as a reference plugin for more implementations.

As a result, the post-submission screen has been extended to show multiple messages. One can imagine a third party bug reporting plugin informing the user "Your bug reference number is #123"

utvalg_240

And the output of the example JSON plugin:

{
  "traceback": null, 
  "stdout": "", 
  "command_line": "echo 'Fail' > /home/hxr/work/galaxy/database/files/000/dataset_94.dat; exit 1;", 
  "user": {
    "username": "test", 
    "deleted": false, 
    "email": "test@domain.edu", 
    "last_password_change": "2017-06-02T19:18:11.278864", 
    "active": false, 
    "model_class": "User", 
    "id": 1
  }, 
  "message": "Testing a bug report", 
  "id": 94, 
  "info": null, 
  "tool_version": "1.0", 
  "tool_xml": "./tools/error.xml", 
  "exit_code": 1, 
  "handler": "main", 
  "stderr": "Fatal error: Exit code 1 ()\n", 
  "email": "hxr@localhost"
}
  • cc @MoffMade, who I believe wanted to implement a "github" plugin.
  • Thanks @jmchilton for a great example of the plugin infrastructure with the metrics infrastructure.
  • It sure is nice being between jobs and having time for fun stuff.

@erasche erasche added this to the 17.09 milestone Jul 12, 2017

@erasche erasche requested a review from jmchilton Jul 12, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

This is a great idea.

Can we use a simple dictionary endpoint and clientside route, similar to @guerler's recent work swapping out grids, to avoid backsliding a bit by adding a new mako? I'd be happy to help with that prior to merge it'd be useful.

And (this one's really not important, just curious!) I'm not super keen on the 'sink' nomenclature. Did you consider any other names?

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

Can we use a simple dictionary endpoint and clientside route, similar to @guerler's recent work swapping out grids, to avoid backsliding a bit by adding a new mako?

We sure could. I have no experience converting the mako to JS. I hope to practice with the job info page at some point soon, maybe thursday or friday (once I'm done packing my house). If I promise to do it before the next release, would you be ok if I refactor this in a separate PR?

And (this one's really not important, just curious!) I'm not super keen on the 'sink' nomenclature. Did you consider any other names?

I hadn't, actually. Source/Sink was the first thing that came to mind. Do you have something you'd prefer?

@bgruening

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@anuprulez could help migrating this. He is an expert in this kind of stuff after it is merged, if this helps.
Great feature @erasche!

@bgruening

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@erasche can I bribe you to add a slide to the admin training after this is merged :)

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

@bgruening was hoping to learn how to do client end stuff with this / job info page, before @anuprulez can get to them ;) and yes, of course, should be documented. that should be a blocking problem with this PR, i'll fix it now.

}
)
for dataset in job.output_datasets:
self.app.error_reports.default_error_sink.submit_report(dataset, job, tool, user_submission=False)

This comment has been minimized.

Copy link
@erasche

erasche Jul 12, 2017

Author Member

@jmchilton is this the right way to do this? I get the impression that it isn't, but I wasn't understanding the lambda expression that I copied from your metrics. I.e. https://github.com/galaxyproject/galaxy/pull/4305/files#diff-218b8eec0cd1aaab08f26181429f4ba0R19

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jul 12, 2017

Member

The job instrumenters you copied this from are per-job-destination. So different job destinations can have different job metrics collected. The default applies to any that aren't specified on a per destination basis - which is likely every destination ever since this wasn't really documented. I'd probably argue the error reporting should be overridable on a per destination basis for the same reason - but since I didn't document it replicating the full complexity of job metrics is not something I'd really require from you.

But to answer the question about the odd line:

    self.error_sinks = collections.defaultdict(lambda: self.default_error_sink)

It is meant to be used like:

    self.app.error_reports.error_sinks[job.destination_id].submit_report(dataset, job, tool, user_submission=False)

In this case, the lambda is just telling the defaultdict which set of error sink plugins to use if no per-destination one has been explicitly set. I'd make that change just to bring it inline with job metrics and because you have the job available right there - it allows us to grow in that direction in the future (per-destination error reporting) - even if you don't add the job conf hooks now.

Ideally everything is overridable at the destination this way. The nature of job destinations is also such that this means it is overridable from dynamic job destinations and on a per-tool basis for instance also.

@erasche erasche added status/WIP and removed status/review labels Jul 12, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

utvalg_241

sentry plugin can now be verbose. (I.e. display a message to the user when it is configured to be run on user-submission).

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Generally speaking, I think we should for new features include new style client endpoints in the initial merge, unless there's pressure to get something in. It doesn't hurt anything for PRs to stay open and continue getting worked on, and we more often 'lose' issues and TODOs than we do open PRs. That said, I'm definitely not a - on this until it's done, I'd just prefer it the other way 'round.

And, maybe just 'Plugin'? Sink just seems overly technical (if completely correct) and harder to grok at a glance, to me putting myself in the shoes of a naive browser of the codebase.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

@dannon all fair. We have a ways to go until 17.09, so I'll work on a client side replacement as well, though that will significantly delay this PR being ready.

I had originally called it ErrorReporter, but I couldn't easily visually distinguish it from "ErrorReports" (i.e. https://github.com/galaxyproject/galaxy/pull/4305/files#diff-218b8eec0cd1aaab08f26181429f4ba0R12) so changed it to sink. ErrorPlugin will be fine, I can do this as well.

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@erasche Awesome, thanks for being understanding, and let me know if I can help out at all! This is great stuff, and a valuable improvement.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

@erasche If you just change that config file extension to .yml - none of your other code would need to change (e.g. this common-workflow-language/cwltool@b19f60d). The plugin framework should just work after you update the default config paths. I made the new workflow scheduler plugins file YAML by default - it is the direction I'd like to head. Are you open to swapping this out - or would you prefer to merge as is?

For name suffix, I prefer Sink to Plugin but am okay with either. Either ErrorCollector or ErrorRespnder probably would have been the name I would have used - but that is hardly an endorsement.

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Maybe 'handlers' instead of 'sinks' or 'plugins'? So, ErrorHandler, EmailErrorHandler, etc.

edit: (just to reiterate, this is not an important issue for me, just struck me as awkwardly named and I was curious -- do whatever seems right to you)

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

(Last day of packing, will respond tonight. Thanks for the great feedback y'all!)

@@ -43,6 +43,7 @@
job_config_file=['config/job_conf.xml', 'job_conf.xml'],
tool_destinations_config_file=['config/tool_destinations.yml', 'config/tool_destinations.yml.sample'],
job_metrics_config_file=['config/job_metrics_conf.xml', 'job_metrics_conf.xml', 'config/job_metrics_conf.xml.sample'],
error_report_file=['config/error_report.xml', 'error_report.xml', 'config/error_report.xml.sample'],

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jul 12, 2017

Member

I'd remove this middle entry - we allow them to sit in $GALAXY_ROOT for some older configs for legacy reasons - they were there before we introduced the config/ directory - we don't want new config files in $GALAXY_ROOT.

@erasche erasche force-pushed the erasche:bug-report-actions branch from 13e2a8a to 1112528 Aug 7, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

Per request, it was made into an API / client side thing. Reused style from the job info page for visual distinctiveness

auswahl_002

I've integrated the BioStars reporting as well, tested, it works.

auswahl_003

As part of that I switched the biostars submission over to use the relatively new HTML error report template since it looks so much nicer.

auswahl_004

@erasche erasche force-pushed the erasche:bug-report-actions branch from 3a8863d to feac532 Aug 7, 2017

@erasche erasche changed the title Refactor bug reporting into plugins Refactor bug reporting into plugins / Remove bug report mako Aug 7, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

This is ready for review :) Rebased against dev.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

Not sure why these tests are failing. @martenson?

erasche and others added some commits Aug 7, 2017

@erasche erasche force-pushed the erasche:bug-report-actions branch from c30aa5a to 4146123 Aug 10, 2017

@jmchilton jmchilton merged commit 6d7d4e9 into galaxyproject:dev Aug 10, 2017

0 of 5 checks passed

api test Test scheduled.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test scheduled.
Details
integration test Test scheduled.
Details
toolshed test Test scheduled.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

Thanks @erasche - this is really cool stuff!

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

people shouldn't have to update anything in their configs when upgrading Galaxy to re-enable this I think. That is good.

I tried to be very careful about this, if I have missed anything I want to be sure to catch it now. This will be a nice feature, but as you've mentioned, I don't want to negatively affect anyone with these changes or cause people to lose bug reports.

It looks like you still get an error box to write an e-mail even if Galaxy knows it isn't going to work. This is a pre-existing problem though - that doesn't need to be fixed with this PR.

The error box's contents will also be included in any other plugin that wants to make use of it. E.g. the sentry plugin will accept a message. So in this way, I think that it is fine for the box to be essentially a permanent fixture. In the email-only world, I agree that it was weird to show the box, knowing ahead of time that it would break.

The only similar case here was if the admin actively disables all error plugins. We could probably stand to show a message if there were no enabled backends for error reporting, but I think that that is a corner case.

The error message was a little nicer before - I got a nice red box and now it is just text on a black background.

The class names changed when I changed the css a bit, and it looks like I forgot to update the email plugin. Good catch, thanks.

The one other thing I would like to see though is the outdated code axed. It looks like messages.mako is added and not needed anymore - as well as the helper that called it. I think this commit jmchilton/galaxy@d9a0492 should be enough if you want to cherry-pick it.

Oooh, awesome, thanks for doing that. Added.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

apparently I never hit submit, sorry about that :) thanks for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.