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

Major refactor of internal Log module #54

Closed
wants to merge 5 commits into from
Closed

Conversation

asenchi
Copy link
Owner

@asenchi asenchi commented Feb 6, 2014

To address #53 this restructures our internal log object. Instead of a module we move to a class that is instantiated upon using #init. This breaks current Scrolls behavior in the following ways:

  • The global_context is no longer mutable, instead we instantiate a class internally inside Scrolls::Logger that contains the global_context. This is to avoid various bad thread behaviors, that previous to Ruby 2.0, were allowed.
  • The result of the above change removes #add_global_context.
  • Currently Scrolls needs to be instantiated to be used (Scrolls.init(options)) however I'd like to work on a method for allowing one to just use Scrolls.log and get sane defaults.
  • Due to this massive change I moved to "single line exceptions" by default since it lowers the amount of data produced by Scrolls when using #log_exception without losing the information.
  • We drop support for Ruby 1.8.7.

I've currently got all tests passing:

projects/scrolls major-refactor! : rbenv local 2.1.0
projects/scrolls major-refactor! : rake
Run options: -v

# Running tests:

TestScrolls#test_add_timestamp = 0.00 s = .
TestScrolls#test_all_the_contexts = 0.00 s = .
TestScrolls#test_construct = 0.00 s = .
TestScrolls#test_context_after_exception = 0.00 s = .
TestScrolls#test_deeply_nested_context = 0.00 s = .
TestScrolls#test_deeply_nested_context_dropped = 0.00 s = .
TestScrolls#test_default_context = 0.00 s = .
TestScrolls#test_default_global_context = 0.00 s = .
TestScrolls#test_default_logging_levels = 0.00 s = .
TestScrolls#test_default_time_unit = 0.00 s = .
TestScrolls#test_level_translation_error = 0.00 s = .
TestScrolls#test_level_translation_fatal = 0.00 s = .
TestScrolls#test_level_translation_unknown = 0.00 s = .
TestScrolls#test_level_translation_warn = 0.00 s = .
TestScrolls#test_log_exception = 0.00 s = .
TestScrolls#test_logging = 0.00 s = .
TestScrolls#test_logging_block = 0.00 s = .
TestScrolls#test_logging_strings = 0.00 s = .
TestScrolls#test_multi_line_exceptions = 0.00 s = .
TestScrolls#test_setting_context = 0.00 s = .
TestScrolls#test_setting_global_context = 0.00 s = .
TestScrolls#test_setting_incorrect_time_unit = 0.00 s = .
TestScrolls#test_setting_syslog_facility = 0.00 s = .
TestScrolls#test_setting_syslog_facility_after_instantiation = 0.00 s = .
TestScrolls#test_setting_time_unit = 0.00 s = .
TestScrolls#test_syslog_facility = 0.00 s = .
TestScrolls#test_syslog_integration = 0.00 s = .
TestScrollsParser#test_parse_bool = 0.00 s = .
TestScrollsParser#test_parse_numbers = 0.00 s = .
TestScrollsParser#test_parse_strings = 0.00 s = .
TestScrollsParser#test_parse_time = 0.00 s = .
TestScrollsParser#test_unparse_constants = 0.00 s = .
TestScrollsParser#test_unparse_nil = 0.00 s = .
TestScrollsParser#test_unparse_time = 0.00 s = .


Finished tests in 0.012021s, 2828.3837 tests/s, 5074.4530 assertions/s.

34 tests, 61 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin13.0]
projects/scrolls major-refactor! : rbenv local 1.9.3
projects/scrolls major-refactor! : rake
Run options: -v

# Running tests:

TestScrolls#test_add_timestamp = 0.00 s = .
TestScrolls#test_all_the_contexts = 0.00 s = .
TestScrolls#test_construct = 0.00 s = .
TestScrolls#test_context_after_exception = 0.00 s = .
TestScrolls#test_deeply_nested_context = 0.00 s = .
TestScrolls#test_deeply_nested_context_dropped = 0.00 s = .
TestScrolls#test_default_context = 0.00 s = .
TestScrolls#test_default_global_context = 0.00 s = .
TestScrolls#test_default_logging_levels = 0.00 s = .
TestScrolls#test_default_time_unit = 0.00 s = .
TestScrolls#test_level_translation_error = 0.00 s = .
TestScrolls#test_level_translation_fatal = 0.00 s = .
TestScrolls#test_level_translation_unknown = 0.00 s = .
TestScrolls#test_level_translation_warn = 0.00 s = .
TestScrolls#test_log_exception = 0.00 s = .
TestScrolls#test_logging = 0.00 s = .
TestScrolls#test_logging_block = 0.00 s = .
TestScrolls#test_logging_strings = 0.00 s = .
TestScrolls#test_multi_line_exceptions = 0.00 s = .
TestScrolls#test_setting_context = 0.00 s = .
TestScrolls#test_setting_global_context = 0.00 s = .
TestScrolls#test_setting_incorrect_time_unit = 0.00 s = .
TestScrolls#test_setting_syslog_facility = 0.00 s = .
TestScrolls#test_setting_syslog_facility_after_instantiation = 0.00 s = .
TestScrolls#test_setting_time_unit = 0.00 s = .
TestScrolls#test_syslog_facility = 0.00 s = .
TestScrolls#test_syslog_integration = 0.00 s = .
TestScrollsParser#test_parse_bool = 0.00 s = .
TestScrollsParser#test_parse_numbers = 0.00 s = .
TestScrollsParser#test_parse_strings = 0.00 s = .
TestScrollsParser#test_parse_time = 0.00 s = .
TestScrollsParser#test_unparse_constants = 0.00 s = .
TestScrollsParser#test_unparse_nil = 0.00 s = .
TestScrollsParser#test_unparse_time = 0.00 s = .


Finished tests in 0.007418s, 4583.4457 tests/s, 8223.2408 assertions/s.

34 tests, 61 assertions, 0 failures, 0 errors, 0 skips

I'd like some review on this approach. If the following people wouldn't mind reviewing I would greatly appreciate it: @gorsuch @dpiddy @eric

We move to a class from a module and on instantiation also
instantiate a class for our global_context that we freeze in
place. This removes the idea of adding to our global_context
once we've instantiated our logger.

The rewrite was to address the new signal/thread handling in
Ruby 2.0. Details can be found in
#53.

I think this also provides us with some flexibility around global
context in the future. Unfortunately this is not backward
compatiable with current Scrolls releases.
@asenchi
Copy link
Owner Author

asenchi commented Feb 6, 2014

Also /cc @slemiere since this reverts his feature in #33.

@asenchi
Copy link
Owner Author

asenchi commented Feb 6, 2014

Heh just realized that I simultaneously supported 1.8.7 in our parser tests and dropped it for the project as a whole.

@spicycode
Copy link

@asenchi Nice. Looking forward to this landing

@gorsuch
Copy link

gorsuch commented Feb 10, 2014

All of this seems reasonable to me and like a step in the right direction. I prefer instantiation.

@gregburek
Copy link

Now that ruby 1.9.3 is in security fixes only mode I am very excited to get this merged and deployed! Do you need any help or alpha testers for this change?

@grantr
Copy link
Contributor

grantr commented Mar 26, 2014

@asenchi can you give an example of the new single line exception log format?

@asenchi
Copy link
Owner Author

asenchi commented Aug 22, 2017

I will be working to refactor this code since it has sat stale for so long. Apologies for my lax behavior here. I cut a new release with the current behavior here in preparation for this work.

asenchi added a commit that referenced this pull request Aug 23, 2017
This commit breaks backwards compatability for Scrolls. It
is a rework/refactor of the initial work done in the
following PR: #54

- The `global_context` is no longer mutable, instead we
instantiate a class internally inside Scrolls::Logger that
contains the `global_context`. This is to avoid various bad
thread behaviors, that previous to Ruby 2.0, were allowed.
- The result of the above change removes `#add_global_context`.
- Currently Scrolls needs to be instantiated to be used
(`Scrolls.init(options)`) however I'd like to work on a
method for allowing one to just use `Scrolls.log` and get
sane defaults.
- Due to this massive change I moved to "single line
exceptions" by default since it lowers the amount of data
produced by Scrolls when using `#log_exception` without
losing the information.
@asenchi asenchi mentioned this pull request Aug 23, 2017
@asenchi
Copy link
Owner Author

asenchi commented Aug 24, 2017

Replaced by #71.

@asenchi asenchi closed this Aug 24, 2017
@asenchi asenchi deleted the major-refactor branch August 24, 2017 20:47
@asenchi asenchi mentioned this pull request Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants