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

Create ASL capture module. #242

Merged
merged 14 commits into from
Apr 7, 2014
Merged

Create ASL capture module. #242

merged 14 commits into from
Apr 7, 2014

Conversation

DarioAhdoot
Copy link
Contributor

A number of notes:

  • The code is basically modified from the link I sent in the previous issue. Not a whole lot has changed.
  • I've added TODO: comments where your attention is required
  • It's not yet integrated with the rest of the Lumberjack framework as it's not yet clear the best way to do so. Also, I'm not sure if it makes sense to forward the messages on to DDLog since if you have a TTY logger or an ASL logger installed, then they will end up getting duplicates and you can potentially end up in an infinite recursion if you have an ASL logger. Really only makes sense for the file logger, or perhaps custom loggers people may write. Not sure the best way to proceed.
  • I'm not that experienced with GCD so I can't attest to the stability of the code. I haven't tested start/stopping the ASL capture module.
  • My Xcode has tabs set to 2 spaces, which is different than yours apparently. Sorry :-)

…he Lumberjack framework as it's not yet clear the best way to do so.
// TODO: We have some options here. One is to call the corresponding DDLog macros based on the log level. However
// we won't get the timestamp of the original NSLog there, which is an important piece of information. Alternatively
// we will have to create a DDLogMessage object and set the timestamp directly, but there is no DDLog interface for
// accepting a DDLogMessage.
Copy link
Member

Choose a reason for hiding this comment

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

Manually creating the log message is the best choice, we can then make + (void)queueLogMessage:(DDLogMessage *)logMessage asynchronously:(BOOL)asyncFlag public and use it.

Alternatively we could add a timestamp parameter to the DDLogMessage's init, and set it no the current time if no timestamp is set.

* Add new init method too DDLogMessage which takes in a timestamp
* Make DDLog queueLogMessage:asynchronously: public
* Add nanoseconds to ASL capture timestamps
* Adjust some logging levels.
* TODO: Need to figure out how to get ddLogLevel into the ASLLogCapture module so it can be passed to the DDLogMessage init method. Perhaps declare it as extern in DDLog.h?
@DarioAhdoot
Copy link
Contributor Author

Hi Ernesto. I've pushed a new commit where I'm calling DDLog queueLogMessage:asynchronously: with a DDLogMessage initialized with the NSLog contents. There are a couple of things left to do which I'm not quite comfortable doing:

  1. Need to get ddLogLevel variable scoped into the ASLLogCapture file. It's needed to pass to the logFlag variable of the DDLogMessage. Not sure of the best way to do that
  2. I'm not sure if we should be calling queueLogMessage synchronously or asynchronously. I left it asynchronous to be safe.
  3. I haven't touched the tag/context values as you suggested as I'm not familiar enough with Lumberjack to know how best to use those as you suggested
  4. I'm not sure of the best place to integrate the DDASLCapture module into the rest of the codebase. More specifically, I'm not sure who should own one, where it should be started/stopped, etc. I figure it's best for you to take care of that, or at least let me know your thoughts on where that should happen
  5. We might want to update the log level filtering on ASL based on the ddloglevel value. Currently we're getting every single ASL log message and relying on the framework to filter. The framework is filtering properly so this may not be an issue.

Thanks,
Dario

@rivera-ernesto
Copy link
Member

  1. Need to get ddLogLevel variable scoped into the ASLLogCapture file. It's needed to pass to the logFlag variable of the DDLogMessage. Not sure of the best way to do that

Maybe a new higher level +logMessage:(DDLogMessage *)message in DDLog would be better than making +queueLogMessage:asynchronously: public (sorry!). Then DDLog can directly overwrite ddLogLevel.

  1. I'm not sure if we should be calling queueLogMessage synchronously or asynchronously. I left it asynchronous to be safe.

We could let the user choose by changing the start method to startAsynchronously:(BOOL)yesOrNo, etc.

  1. I haven't touched the tag/context values as you suggested as I'm not familiar enough with Lumberjack to know how best to use those as you suggested

Tag is for now simply ignored so we can use that. I can do a separate pull request for this and in the mean time just the ASL logger should be skipped when capturing ASL logs.

  1. I'm not sure of the best place to integrate the DDASLCapture module into the rest of the codebase. More specifically, I'm not sure who should own one, where it should be started/stopped, etc. I figure it's best for you to take care of that, or at least let me know your thoughts on where that should happen

We can do that or you could implement it as all + class methods just like DDLog does. Also a separate file like now is perfect I think.

  1. We might want to update the log level filtering on ASL based on the ddloglevel value. Currently we're getting every single ASL log message and relying on the framework to filter. The framework is filtering properly so this may not be an issue.

Maybe to keep it simple we could have a separate +setCaptureLogLevel: directly in your class, you can still use the levels defined in DDLog.

Thanks to you! Looking forward to start using it ;)

* Allow client to specify whether ASL logs get dispatched synchronously or asynchronously
@DarioAhdoot
Copy link
Contributor Author

Hi Ernesto. I've just pushed another commit which integrates your suggestions for items 2 and 4. For item 1, I'm not clear exactly how that would help as DDLog.m still doesn't have access to ddLogLevel. I'll leave items 1 and 3 up to you. For item 5, I've decided to leave it and use allow all ASL Log messages to be captured and allow the framework to filter.

@rivera-ernesto
Copy link
Member

Sure. I'll try to address the remaining points. Thanks again!

@DarioAhdoot
Copy link
Contributor Author

Pull request with your changes included.

@rivera-ernesto
Copy link
Member

@Diarrhio does it work well for your usages?

@bpoplauschi what do you think about using DDASLLoggerIgnoreLogMessageTag to ignore ASL messages?

@DarioAhdoot
Copy link
Contributor Author

Yup.

@rivera-ernesto
Copy link
Member

Ok, merging this myself.

Let's try to use pod 'CocoaLumberjack', :head to test everything before we make a new release.

rivera-ernesto pushed a commit that referenced this pull request Apr 7, 2014
Create ASL capture module.
@rivera-ernesto rivera-ernesto merged commit 888cd3f into CocoaLumberjack:master Apr 7, 2014
@bpoplauschi
Copy link
Member

@rivera-ernesto: I just saw the question about DDASLLoggerIgnoreLogMessageTag, sounds good to me.

About testing, I think we lack some good unit tests that we can run after a big change like this or before a release. Will add that to our 2.0 list.

@DarioAhdoot
Copy link
Contributor Author

I've done some preliminary tests with :head and things seem to be working well. Will file an issue if I find anything.

@rivera-ernesto
Copy link
Member

@Diarrhio One last thing before making a release. There's an Analyzer warning here.

DDASLLogCapture.m:136:124: Variable 'notifyToken' is uninitialized when captured by block

    int notifyToken;  // Can be used to unregister with notify_cancel(). // <- We need to set a default value here!
    notify_register_dispatch(kNotifyASLDBUpdate, &notifyToken, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^(int token)
    {
      // At least one message has been posted; build a search query.
      @autoreleasepool
      {
        // ...

        if(_cancel)
        {
          notify_cancel(notifyToken);
          return;
        }
      }
    });

@rivera-ernesto rivera-ernesto mentioned this pull request Apr 14, 2014
1 task
@rivera-ernesto
Copy link
Member

Silenced the warning just by setting int notifyToken = 0 here.

@bpoplauschi bpoplauschi added this to the 1.9.0 milestone May 16, 2014
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

3 participants