Opened 7 years ago

Closed 6 years ago

#3095 closed enhancement (complete)

Generic traceback handling

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20130917
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The goal is to wrap all top-level makes of python into „catchall“ exception handling. We store the traceback to a file for easier bugreport and log the error as FATAL. This way, we don't clutter the output with tracebacks and have proper logging for all the errors.

Be sure to use except Exception as e:, not just except:, because then we would catch the fake control exceptions (like the one generated by Ctrl+C).

Subtickets

Change History (10)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130903

comment:3 Changed 7 years ago by muks

Check if #3003 can be closed too when this ticket is done.

comment:4 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:5 follow-ups: Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

The ticket is ready for review. However, I discovered a small catch there. It
does handle exceptions if the main function already started. But not when the
import statements are still being evaluated. So, this means it'll still
traceback the usual way when we are missing some of the libraries or if there's
a syntax error. I don't know if it is a real problem and I don't know how to
fix that anyway, so we'll probably have to live with that.

Proposed changelog:

[func]		vorner
If there's an exception not handled in the code, it is now stored in a
temporary file and properly logged, instead of dumping to stderr.

comment:6 in reply to: ↑ 5 Changed 6 years ago by muks

Replying to vorner:

The ticket is ready for review. However, I discovered a small catch there. It
does handle exceptions if the main function already started. But not when the
import statements are still being evaluated. So, this means it'll still
traceback the usual way when we are missing some of the libraries or if there's
a syntax error. I don't know if it is a real problem and I don't know how to
fix that anyway, so we'll probably have to live with that.

I think this is not a problem as if import statements fail, something is wrong with the basic installation itself and that is the best way for it to fail.

comment:7 Changed 6 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review.

comment:8 in reply to: ↑ 5 Changed 6 years ago by muks

Replying to vorner:

Proposed changelog:

[func]		vorner
If there's an exception not handled in the code, it is now stored in a
temporary file and properly logged, instead of dumping to stderr.

Maybe we should elaborate here that "If there's an exception not handled in a Python BIND10 component..."

comment:9 follow-up: Changed 6 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

Here is my review of the branch:

  • It seems to pass make distcheck.
  • In util_messages.mes, the following comment must be fixed:
    +# No namespace declaration - these constants go in the global namespace
    +# of the libddns_messages python module.
    
  • I think it's better to rename UNHANDLED_EXCEPTION message id to PYTHON_UNHANDLED_EXCEPTION, as this message id is used specifically for Python components.
  • Do we want to use this traceback handler for dbutil and loadzone too? I'm just raising this point; I don't have a strong preference.
  • I have pushed some minor commits which I didn't feel needed more discussing. Please check if they are okay.

Apart from these, the branch is perfect, so please go ahead and merge
after addressing the above.

comment:10 in reply to: ↑ 9 Changed 6 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed

Hello

Replying to muks:

  • Do we want to use this traceback handler for dbutil and loadzone too? I'm just raising this point; I don't have a strong preference.

I think we do want it, because dumping of traceback to stderr is not nice under any circumstances.

Anyway, thank you, merged.

Note: See TracTickets for help on using tickets.