Wire up client to send errors to http server / Change 'shows details' button to 'report to TripleA' #4687

DanVanAtta · 2019-02-16T02:58:22Z

Overview

First two commits are mostly house-keeping/API changes, third commit summarizes the bulk of this update:

Wire up client to send errors to http server

User facing change where on the error message pop-up window, instead of
having a 'Show Details' button (that would open the console), we now
have a 'Report to TripleA' button that opens up a form window. The form
window has one text field where a user can enter details (optional), and
click 'upload' to then send to http server.

Notable code/design changes:
- LogRecord maybe having an exception is a bit tricky, there is a lot of
logic in formatting the title+body to have data extracted from log
record.
- Error report window is essentially re-written, focusing on case where
we have a LogRecord object and user input is optional.
- Error report window model has new design/contract where it interacts
with the UI only through interface. The UI has a 'bind' method to let it
attach model methods to UI components. This avoids a circular dependency
between UI and model, and model and the view interface have no JavaFx or
Swing dependencies, so we can swap out different view interfaces between
the two without changing model.

Functional Changes

'Show details' button on the error pop-up window that we show when there is a log mesage that triggers a pop-up or an uncaught exception is replaced with a 'upload to triplea button'
http server http client, feign, logging level is reduces to no longer log http header values

Manual Testing Performed

started an http server and injected an uncaught exception and added logging statements to headedGameRunner startup to verify. Example issues created can be viewed at: http://github.com/triplea-game/test/issues. http://github.com/triplea-game/test/issues/51 + http://github.com/triplea-game/test/issues/47 are perhaps good examples to look at.

Before & After Screen Shots

Before

Error message pop-up dialog, note the 'Show Details' button:

After

When there is an error message the 'show details' button is now replaced:

Clicking 'show details' opens this form window that has a hover text. The submit button can be clicked and error report will be sent with the system + log record exception or message data:

If the http server has a problem (simulated by having incorrect github API token), then the client will see this error message and the upload form will remain open:

Happy case where http server uploads, this response will be returned with clickable link:

Example Error Report

triplea-game/test#54

Additional Review Notes

Related design discussion: Error Report - User Form Data #4589
Once this is landed, the next and last piece for error reporting is server side throttling.

To demo:

add a log.severe('severe') or throw an uncaught excpetion in HeadedGameRunner.java

The http server will not be there and it'll cause an error, but it'll be the full flow minus the success message. To get an http server running:

set the game override for http server to be localhost
take a moment to find and set the github API key (logged in Triplea secrets)
start up http server
launch HeadedGameRunner with injected error or log statement.

The flow is still pretty interesting even without the http-server running.

The 'border' method now becomes an overload, bit more obvious that it is a simplified version of 'border(BorderBuilder)'

Enables a swing property in JTextAreaBuilder that will be used in an upcoming change.

User facing change where on the error message pop-up window, instead of having a 'Show Details' button (that would open the console), we now have a 'Report to TripleA' button that opens up a form window. The form window has one text field where a user can enter details (optional), and click 'upload' to then send to http server. Notable code/design changes: - LogRecord maybe having an exception is a bit tricky, there is a lot of logic in formatting the title+body to have data extracted from log record. - Error report window is essentially re-written, focusing on case where we have a LogRecord object and user input is optional. - Error report window model has new design/contract where it interacts with the UI only through interface. The UI has a 'bind' method to let it attach model methods to UI components. This avoids a circular dependency between UI and model, and model and the view interface have no JavaFx or Swing dependencies, so we can swap out different view interfaces between the two without changing model.

codecov-io · 2019-02-16T03:08:48Z

Codecov Report

Merging #4687 into master will increase coverage by 0.07%.
The diff coverage is 37.34%.

@@             Coverage Diff             @@
##             master   #4687      +/-   ##
===========================================
+ Coverage     23.22%   23.3%   +0.07%     
- Complexity     6213    6221       +8     
===========================================
  Files           873     870       -3     
  Lines         70483   70389      -94     
  Branches      11242   11243       +1     
===========================================
+ Hits          16369   16401      +32     
+ Misses        52078   51949     -129     
- Partials       2036    2039       +3

Impacted Files	Coverage Δ	Complexity Δ
...client/github/issues/GithubIssueClientFactory.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...rver/reporting/error/ErrorUploadConfiguration.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...gy/engine/framework/startup/ui/MetaSetupPanel.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ine/framework/map/download/DownloadMapsWindow.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...c/main/java/games/strategy/debug/ErrorMessage.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...trategy/engine/lobby/client/ui/TimespanDialog.java	`27.58% <0%> (ø)`	`3 <0> (ø)`	⬇️
...ne/framework/startup/ui/panels/main/MainPanel.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...games/strategy/triplea/ui/FindTerritoryDialog.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...gy/debug/error/reporting/StackTraceReportView.java	`0% <0%> (ø)`	`0 <0> (?)`
.../error/reporting/ConfirmationDialogController.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfd00ee...ed10bfa. Read the comment docs.

RoiEXLab

Well there's not too much I can say about this.
The code looks fine (except for string building of course, that looks always messy)
Also I hesitate a bit to press the button, because this fundamentally changes the way we do debugging (with user problems)

...-core/src/main/java/games/strategy/debug/error/reporting/StackTraceErrorReportFormatter.java

DanVanAtta · 2019-02-17T20:12:06Z

Sincere thank you for looking over the code @RoiEXLab despite any delivery criticisms.

It's a bit late to be doing UX design, but I have been considering whether to add 'report' as a 3rd button and keeping the 'show details' button.

A subtle benefit here is an error report can now be done with zero key-strokes and two button clicks. A pretty low bar for sending us an error report. To re-iterate, just clicking the 'report' and 'send' buttons will upload the stack-trace.

The nail in the 'show details' coffin for me is the unnecessary control, adding UI elements subtracts from the other UI elements. The stack-trace is for dev's, we can see that without a button to 'show details'.

RoiEXLab

No more objections from my side.

RoiEXLab · 2019-02-17T20:45:58Z

...-core/src/main/java/games/strategy/debug/error/reporting/StackTraceErrorReportFormatter.java

    return Arrays.stream(stackTrace)
        .map(StackTraceElement::toString)
-        .toArray(String[]::new);
+        .collect(Collectors.joining("\n\n"));


I'm curious where the additional newline comes from.

Good eye, fixed: ed10bfa

…e space

tvleavitt · 2019-02-18T01:24:46Z

Dan, I may be odd as an end user, but I'm always a bit antsy about sending info out to a third party that I can't examine in detail. I've seen error reports that gather a remarkable amount of information, some of which might actually have privacy concerns (such as a list of every other process running on the machine, for example). I wonder if I'm alone in this. Obviously, most people won't have the slightest clue about how to evaluate what they're seeing (in large part, I'm one of them, in fact), so there's not a lot of practical / actionable benefit from being able to see details... on the other hand, we're asking users to blindly trust that we're not gathering anything problematic, and that concerns me as a general principle, and I wonder if other folks out there might have the same reaction. Regards, Thomas Leavitt

…

On Sun, Feb 17, 2019 at 12:12 PM Dan Van Atta ***@***.***> wrote: Sincere thank you for looking over the code @RoiEXLab <https://github.com/RoiEXLab> despite any delivery criticisms. It's a bit late to be doing UX design, but I have been considering whether to 'report' as a 3rd button and keeping the 'show details' button. That introduces other problems. Ultimately the stack-trace is visible by any way a dev would launch the game, and is fundamentally most useful to a dev who 'can' open the console anyways. A subtle benefit is that the error report can now be down with zero key-strokes and two button clicks, that is a pretty low bar for sending us an error report. To re-iterate, just clicking the 'report' and 'send' buttons will upload the stack-trace. The nail in the 'show details' coffin for me is the unnecessary control, adding UI elements subtracts from the other UI elements. We ideally want most users to click 'report' and upload the error, encouraging that as much as possible is the objective. We do want to see an increase in any stack-traces being reported to us. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4687 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEPTgTl5O4bikGvljqETrXDHexlPCuWlks5vObeWgaJpZM4a-voU> .

-- Thomas Leavitt Internet enabled since 1990

DanVanAtta · 2019-02-18T01:51:58Z

@tvleavitt I originally had in the last iteration a number of confirmations/warnings/previews. For the sake of getting this out the door, it was redesigned to have a much simpler more focused mandate that could land more easily.

Adding back in a 'preview' button would not be too difficult, particularly if it is a modal dialog. On the other hand, all info uploaded is viewable from the issue ticket created. I wonder if instead we could just message via tooltip that the error report with all details uploaded will be viewable after upload? So should this be fixed with a preview button, or perhaps better tooltip messaging?

DanVanAtta · 2019-02-22T19:49:14Z

I created a tracking card to update the preview/messaging treatment: https://github.com/triplea-game/triplea/projects/4#card-17997555; @tvleavitt I'll mention you on the PR update that tries to improve on that point.

DanVanAtta added 3 commits February 15, 2019 18:15

Method name update, borderEmpty -> border

124bec3

The 'border' method now becomes an overload, bit more obvious that it is a simplified version of 'border(BorderBuilder)'

Add 'toolTip' property to JTextAreaBuilder

8ff09bf

Enables a swing property in JTextAreaBuilder that will be used in an upcoming change.

DanVanAtta added the PR: Feature Add label Feb 16, 2019

This was referenced Feb 16, 2019

Add 'toolTip' property to JTextAreaBuilder #4682

Closed

Method name update, borderEmpty -> border #4683

Closed

RoiEXLab reviewed Feb 17, 2019

View reviewed changes

...-core/src/main/java/games/strategy/debug/error/reporting/StackTraceErrorReportFormatter.java Outdated Show resolved Hide resolved

DanVanAtta assigned ron-murhammer and ssoloff Feb 17, 2019

Simplify: Use Collectors.joining over String.join

36fe92f

RoiEXLab approved these changes Feb 17, 2019

View reviewed changes

Change: Single space the stack trace in error report instead of doubl…

ed10bfa

…e space

DanVanAtta merged commit 28e6c53 into triplea-game:master Feb 22, 2019

DanVanAtta deleted the error-reporting branch February 22, 2019 19:49

ssoloff unassigned ron-murhammer and ssoloff Feb 23, 2019

DanVanAtta mentioned this pull request Feb 25, 2019

Improve: Error reporting user preview and help text #4713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wire up client to send errors to http server / Change 'shows details' button to 'report to TripleA' #4687

Wire up client to send errors to http server / Change 'shows details' button to 'report to TripleA' #4687

DanVanAtta commented Feb 16, 2019 •

edited

Loading

codecov-io commented Feb 16, 2019 •

edited

Loading

RoiEXLab left a comment •

edited

Loading

DanVanAtta commented Feb 17, 2019 •

edited

Loading

RoiEXLab left a comment

RoiEXLab Feb 17, 2019

DanVanAtta Feb 17, 2019

tvleavitt commented Feb 18, 2019 via email

DanVanAtta commented Feb 18, 2019

DanVanAtta commented Feb 22, 2019

Wire up client to send errors to http server / Change 'shows details' button to 'report to TripleA' #4687

Wire up client to send errors to http server / Change 'shows details' button to 'report to TripleA' #4687

Conversation

DanVanAtta commented Feb 16, 2019 • edited Loading

Overview

Functional Changes

Manual Testing Performed

Before & After Screen Shots

Before

After

Example Error Report

Additional Review Notes

To demo:

codecov-io commented Feb 16, 2019 • edited Loading

Codecov Report

RoiEXLab left a comment • edited Loading

Choose a reason for hiding this comment

DanVanAtta commented Feb 17, 2019 • edited Loading

RoiEXLab left a comment

Choose a reason for hiding this comment

RoiEXLab Feb 17, 2019

Choose a reason for hiding this comment

DanVanAtta Feb 17, 2019

Choose a reason for hiding this comment

tvleavitt commented Feb 18, 2019 via email

DanVanAtta commented Feb 18, 2019

DanVanAtta commented Feb 22, 2019

DanVanAtta commented Feb 16, 2019 •

edited

Loading

codecov-io commented Feb 16, 2019 •

edited

Loading

RoiEXLab left a comment •

edited

Loading

DanVanAtta commented Feb 17, 2019 •

edited

Loading