Allow full error log

[nimrodshn]

Currently we truncate (after 200 characters) the log error / warning that was received when connecting to a provider - In this patch we discard this action in favor of dealing with the full log in the UI.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1506718

cc: @cben @serenamarie125

[nimrodshn]

@miq-bot add_label bug, gaprindashvili/yes

[himdel]

Ping @jrafanie .. you seem to be the original author behind the "truncate error messages to 200 chars" change. Guessing there may have been a reason to do that?

[jrafanie]

@nimrodshn This also affects logging and we store this string in the authentications table in the status_details column. We truncate it because error responses from providers can be huge. We've had may situations where a repeated error condition on login would fill the log or not fit in the varchar column.

I'd be ok with a few different solutions:

• extending the truncate length to fit a reasonable error message to fix your bug (while still fitting into the status_details column)
• Or move the truncate to the log line and where we save the status_details and all callers of authentication_check

[nimrodshn]

If I were to truncate in different locations like you suggested, how much to truncate in each place?

• Log file (I was thinking of doubling the allowed characters to 400)

• status_details column (I was thinking of keeping it the same here)

• all callers to authentication_check (I was doubling the allowed characters to 400)

[jrafanie]

@nimrodshn If you can reliably create an authentication failure error message on your provider > 500 characters, I would avoid truncating here and instead truncate to 400 or 500 in the log line, saving status_details into authentications, and other callers and verify it fixes your issue while not raising an error or silently failing.

[jrafanie]

@nimrodshn The other thing to keep in mind is the length might have been truncated for easier presentation in the UI so it might look strange if various places in the UI don't give enough visual space to show it.

[cben]

About authentications.status_details column it's currently not visible in UI, that's why no point making it longer. (provider details only shows "Error"/"Success" status & how long ago)

if 400 vs 200 is as much as we're willing to raise, not worth the effort IMHO.
I was hoping for several KB, at least for UI.

The other thing to keep in mind is the length might have been truncated for easier presentation in the UI so it might look strange if various places in the UI don't give enough visual space to show it.

Right, immediately showing a screenful of garbage in UI wouldn't look pretty, but there should be something to click to get more info (which the UI PR tries to do, implementation details under discussion). See my comment ManageIQ/manageiq-ui-classic#2862 (comment) for my perspective why when adding a provider doesn't work, and you don't even have any details to diagnose/google/ask for help, it's a miserable experience.

[jrafanie]

About authentications.status_details column it's currently not visible in UI, that's why no point making it longer. (provider details only shows "Error"/"Success" status & how long ago)

@cben I believe it's shown when you hover over the authentication checkmark/X in the provider screen. There's a popup that shows the error for failures.

[jrafanie]

Maybe here @cben ? https://github.com/ManageIQ/manageiq-ui-classic/blob/2f270bb4f872d19559f348f8260186db1f2bfb45/app/helpers/textual_summary_helper.rb#L200

[nimrodshn]

@jrafanie

If you can reliably create an authentication failure error message on your provider > 500 characters, I would avoid truncating here and instead truncate to 400 or 500 in the log line

I did write a small server that does just this (return a really long response..)
Where exactly is 'here' though?

Even still that doesn't seem to address @cben 's issues... and If we do allow, as a compromise, for the error log to propagate, only truncating at the log file line and the status_details column we can somehow handle it gracefully in the UI - what do you think?

[nimrodshn]

@miq-bot add_label wip

[jrafanie]

I did write a small server that does just this (return a really long response..)
Where exactly is 'here' though?

"here" means the line change in this PR (remove the truncate call like you did but truncate it in those other areas)

[nimrodshn]

@jrafanie Now is it OK with you? are there any more places I should truncate the err message?

[jrafanie]

@nimrodshn it seems ok, what test scenarios did you run? Note, if there's any customization in the UIs for each provider that calls authentication_check, they'll be affected by this change.

[nimrodshn]

@jrafanie The basic scenario I tried to solve was when a user adds a new provider he gets a truncated error message. With this PR he would get a longer message which will somehow be handled gracefully in the UI (not yet implemented).
Are there any other use cases you think I should consider?

Screenshot:

[jrafanie]

@nimrodshn I'm ok with this. The backend side is covered by the truncate for the column and log line so as long as @himdel is fine with a possibly enormous error message string being returned to all provider UIs, then I'm good with merging.

[himdel]

@jrafanie No issue from me 👍

UI-side, we talked about having a opt-in option to add_flash that would make the UI only show the first line until the user expands the message.

So assuming no-multimegabyte responses... :)

[himdel]

(That said, maybe it would be good to wait with backporting to gaprindashvili until the UI PR is merged as well, so that we don't introduce a temporary state where the user could get the whole error.)

[jrafanie]

Sounds good to me. @nimrodshn Is the ui classic PR standalone? Can we merge that and backport it first? Then un-WIP this and merge and backport?

[nimrodshn]

@jrafanie Yes that's exactly the plan. I just need to give the UI PR few final touches before it's done.

[nimrodshn]

@jrafanie just as a lower bound I have added another truncate on authentication_check as suggested by @cben in the UI PR.

[miq-bot]

Checked commit nimrodshn@d45e508 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

[cben]

👍

[cben]

Maybe here @cben ? https://github.com/ManageIQ/manageiq-ui-classic/blob/2f270bb4f872d19559f348f8260186db1f2bfb45/app/helpers/textual_summary_helper.rb#L200

Hmm, indeed, textual_authentications shows status_details on hover.
There is also textual_authentications_status — used by containers provider details, and apparently nothing else :( — that shows status.
https://github.com/ManageIQ/manageiq-ui-classic/blob/f83b8f6ba9/app/helpers/textual_mixins/textual_authentications_status.rb#L37

@nimrodshn let's also extend status_details then.
@jrafanie how long is reasonable to store in DB?

[jrafanie]

@cben we check our ems/provider authentications on a schedule every hour I believe, so typically, I'd expect us to persist the status_details at most until the next scheduled check and even then it will be replaced with a "new" status.

[nimrodshn]

@jrafanie UI PR was merged. So, unless any objections this is ready 😄 . Do we need to wait for the UI PR to be backported?

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit f50d518d3a2ef6f318dfa70127d4f2a6382c8416
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Wed Dec 20 11:49:00 2017 -0500

    Merge pull request #16550 from nimrodshn/allow_full_error_log
    
    Allow full error log
    (cherry picked from commit 0e33d1ddc72d3f2a35650e94d281c391e5f04cf4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530645