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.

Sign up for GitHub

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

Jump to bottom

Fixes #27036 - improve logging of error backtraces #658

Merged
merged 1 commit into from Mar 4, 2020
Merged

Fixes #27036 - improve logging of error backtraces #658

merged 1 commit into from Mar 4, 2020

Conversation

lzap
Copy link
Member

@lzap lzap commented Jun 12, 2019

Looks like it's not visible at all.

@lzap
Copy link
Member Author

lzap commented Jun 14, 2019 •
edited

Ok improved it, actually regression made in: 8652a7e

2019-06-14T08:56:28 9302404b [E] TEST
2019-06-14T08:56:28 9302404b [W] Error details for TEST: <StandardError>: Test

It's slightly more readable now.

After IRC discussion with @ekohl we agreed that adding support for try method from Rails will not hurt, it clearly makes code easier to read. After we upgrade to 2.3+ we can drop this.

@lzap lzap changed the title Fixes #27036 - improve libvirt DHCP module error Fixes #27036 - improve logging of error backtraces Jun 14, 2019
@lzap
Copy link
Member Author

lzap commented Jun 14, 2019

Rebased, in case you can't boot smart-proxy with "daemon" dependency error - rebase. It was an issue we've fixed this morning.

ekohl
ekohl reviewed Jun 14, 2019
View reviewed changes
lib/try-patch.rb Outdated
@@ -0,0 +1,50 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

Given there's only one usage of this, I'd prefer to hold off on this. If you do have a larger block of code that uses it, then I'd be happy to include it.

@lzap
Copy link
Member Author

lzap commented Jun 20, 2019

Removed, rebased.

ekohl
ekohl reviewed Jun 21, 2019
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Could you add a test for add? This is complicated (for historical reasons) and I'd feel better if it had tests.

@lzap
Copy link
Member Author

lzap commented Jul 22, 2019

Rebased, added tests.

@lzap
Copy link
Member Author

lzap commented Aug 6, 2019

Fixed rubocop issue, ready for rereview.

ekohl
ekohl reviewed Aug 6, 2019
View reviewed changes
modules/dhcp_libvirt/subnet_service_initializer.rb
@@ -37,7 +37,7 @@ def parse_config_for_subnets
raise Proxy::DHCP::Error("Only one subnet is supported") if ret_val.size > 1
ret_val
rescue Exception => e
msg = "Unable to parse subnets XML"
msg = "Unable to parse subnets XML: #{e}"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the , e to logger.error already add this information?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but apparently I want the error message to be returned to the client. Our error reporting is sometimes very weak.

Copy link
Member

Choose a reason for hiding this comment

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

Would logger.exception be useful here (and later in the file)?

Copy link
Member

Choose a reason for hiding this comment

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

Is the specific exception message still needed when using logger.exception? I think that already formats the message to include the specific exception message.

tbrisker
tbrisker approved these changes Nov 28, 2019
View reviewed changes
Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Any further comments @ekohl? lgtm except for a super minor comment

lib/proxy/log_buffer/decorator.rb Outdated
backtrace = backtrace.respond_to?(:join) ? backtrace.join("\n") : backtrace
# accepts backtrace, exception and simple string for historical reasons
backtrace = if exception_or_backtrace.is_a?(Exception) && !exception_or_backtrace.backtrace.nil?
exception_or_backtrace.message + ': ' + exception_or_backtrace.backtrace.join("\n").to_s
Copy link
Member

Choose a reason for hiding this comment

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

not crucial but the .to_s here is redundent

@lzap
Copy link
Member Author

lzap commented Dec 2, 2019

Rebased.

ekohl
ekohl approved these changes Dec 2, 2019
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Would appreciate to know how the answer to my question but overall 👍 and can be merged regardless of the answer.

modules/dhcp_libvirt/subnet_service_initializer.rb
@@ -37,7 +37,7 @@ def parse_config_for_subnets
raise Proxy::DHCP::Error("Only one subnet is supported") if ret_val.size > 1
ret_val
rescue Exception => e
msg = "Unable to parse subnets XML"
msg = "Unable to parse subnets XML: #{e}"
Copy link
Member

Choose a reason for hiding this comment

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

Would logger.exception be useful here (and later in the file)?

@lzap
Copy link
Member Author

lzap commented Dec 2, 2019

I can't comment in the file, so let me put it here. I can add that, this code is probably pre-exception helper. Rebased.

@tbrisker
Copy link
Member

tbrisker commented Mar 4, 2020

please rebase @lzap

@lzap
Copy link
Member Author

lzap commented Mar 4, 2020

Rebased.

@tbrisker
Copy link
Member

tbrisker commented Mar 4, 2020

Thanks @lzap !

@tbrisker tbrisker merged commit d5f5bca into theforeman:develop Mar 4, 2020
@lzap lzap deleted the libvirt-errors-27036 branch March 4, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

@tbrisker tbrisker tbrisker approved these changes

Assignees

@tbrisker tbrisker

Labels
Needs testing
Projects
None yet
Milestone
No milestone
4 participants
@lzap @tbrisker @ekohl @theforeman-bot