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

custom dispatcher / AppEngine #39

Merged
merged 5 commits into from Oct 9, 2013
Merged

custom dispatcher / AppEngine #39

merged 5 commits into from Oct 9, 2013

Conversation

goodsign
Copy link
Collaborator

@goodsign goodsign commented Oct 9, 2013

I want to use seelog in a package that I want to use on AppEngine and other apps.
I would like to set a logger like explained in https://github.com/cihub/seelog/wiki/Writing-libraries-with-Seelog

I would like to set a logger with a custom dispatcher which writes to the appengine.Context.Criticalf/Errorf/Warningf/Infof/Debugf() methods.

Is there a way to provide a custom dispatcher to a logger?

@goodsign
Copy link
Collaborator

Hi zoechi!

Please check this issue:
#36

This was a similar issue, where issue creator was going to wrap seelog for appengine and asking for additional features for changing log context level. It is already implemented in master (latest rev). There was an example gist there.

Maybe we should create a wiki page in future with examples of appengine integration.

About custom dispatcher: currently there is no such feature, but you can always simply wrap seelog itself.

@goodsign
Copy link
Collaborator

But if you are asking for something like

seelog.LoggerFromWriterWithMinLevel (as in the wiki page above), but with a more complicated receiver interface instead of io.Writer, like:

type LevelWriter interface {
    WriteWithLevel(message string, level LogLevel)
}

then it's perfectly possible. This way you could "wrap" AppEngine logger with seelog instead of wrapping seelog. This way you could treat AppEngine logger as a receiver. This is also a possible way. Let us know what you think!

@zoechi
Copy link
Author

zoechi commented Sep 15, 2013

Hi goodsign,
your response time is awesome!

Yes, it was my intention to wrap AppEngine logger with seelog.
I would like to use some features of seelog with appengine (caller information, formatting, filter, ...).

But I need an interface where the writer is called with more information (e.g. loglevel) like you suggested.

I tried to inject a custom dispatcher because this interface supports the required information.
But this is somewhat complicated because the related interfaces are private and I didn't want to change to much in seelog because I don't know it too well yet.

As I understand your response, there is no such option yet but would be easy to create?
I would really appreciate it if you could extend seelog a bit for this use case.

@goodsign
Copy link
Collaborator

Yes, and personally I wouldn't like to make all this stuff public, because it will make seelog too complicated. But such a feature seems to be pretty common and it may be implemented.

I'll think about it for some time as I need to find what could be the best implementation. Stay tuned!

@goodsign
Copy link
Collaborator

Hi zoechi,

The proposal is like this:

We need to be able to use all seelog functionality (using configuration files) together with the new custom receiver. Thus a new special <custom> receiver will be introduced. It could be used in config like any other receiver:

<seelog>
    <outputs>
        <splitter formatid="format1">
            <file path="log.log"/>
            <file path="log2.log"/>
        </splitter>
        ...
        <custom name="myCoolAppEngineReceiver"/>
    </outputs>
    <formats>
        ...
    </formats>
</seelog>

Then in code you will be able to attach a custom receiver by name to the created logger like this:

logger, err := log.LoggerFromConfigAsFile("seelog.xml")

if err != nil {
    return err
}
err = logger.AttachCustomReceiver("myCoolAppEngineReceiver", myAppReceiver)
if err != nil {
    return err
}

log.ReplaceLogger(logger)

where 'myAppReceiver' must implement the custom receiver interface like

type CustomReceiverInterface interface {
    ReceiveMessage(message string, level LogLevel, context LogContext)
}

What do you think?

@zoechi
Copy link
Author

zoechi commented Sep 17, 2013

Hi goodsign,

yours is a nice solution.

Another approach that comes to my mind would be to make a function available to register receivers

func log.RegisterReceiver(name string, receiver CustomReceiver) 

and then let the config reference those receiver names.
I'm fine with both approaches.

@zoechi zoechi closed this Sep 17, 2013
@zoechi zoechi reopened this Sep 17, 2013
@goodsign
Copy link
Collaborator

Yeah, that's a good idea! To make it 'package-level' in style of standard gob.Register. Feels more idiomatic for me. I'm going to take a small pause to think about it and then I'll implement that.

Also, in addition to config, I think some func like 'seelog.LoggerFromWriterWithMinLevel' is needed. Like seelog.LoggerFromCustomReceiver. I'll think about that too.

@goodsign
Copy link
Collaborator

Hi zoechi,

After a small pause I've finally realized how I'd like to see the implementation of the feature. I've implemented the first prototype on the 'gs_issue_39' branch (latest commit).

Now could you please checkout gs_issue_39 and try the two new features in your real projects. I'd like you to tell how it goes, if there are any problems, maybe some comments somewhere are not clear, or anything like that.

When you accept the implementation, I'll proceed with further inspections, pull-requests, tagging, merging, updating wiki and other stuff.

The two features to try are:

1. Using seelog without xml config file as a full proxy to a custom receiver

To do this, basically you need to create a type implementing CustomReceiver (new type I added), instantiate it yourself and call LoggerFromCustomReceiver just like you called LoggerFromWriterWithMinLevel.

2. Using custom receivers in seelog config file together with all the other features

To do this, you also need to create a type implementing CustomReceiver, but instead of instantiating it, you call RegisterReceiver("yourname", &yourType{}). After that, you can use <custom name="yourname"/> nodes in the config just like any other receiver.

If you need any data, that you want to pass in a config file, that must be then used in your custom receiver, you use attributes starting with "data-". Those are then passed to the CustomReceiver.AfterParse argument (XmlCustomAttrs).

So, if you write a config line like

<custom name="myname" data-somedata="12345" data-other="abcdef"/>

then when your receiver is instantiated, AfterParse is called with XmlCustomAttrs map filled with
"somedata":"12345"
"other":"abcdef"

BTW As with any other receiver, you may also use "formatid" in the <custom> node, if you need to specify format just for it.

@zoechi
Copy link
Author

zoechi commented Sep 22, 2013

Awesome!
I'm not sure if I find time tomorrow to try it.
I send feedback ASAP
Thanks
Günter

@zoechi
Copy link
Author

zoechi commented Sep 24, 2013

Hi seelog,

LoggerFromCustomReceiver works and is really easy - awesome!!

But I can't use RegisterReceiver in AppEngine, my reasoning about that was flawed, sorry about that.
I have to use a context object (unique for each http request) when calling AppEngine logging methods in my custom receiver, so reusing a registered receiver is not a good idea.

The question now is, how can I pass config settings to seelog when using LoggerFromCustomReceiver.

Günter

@goodsign
Copy link
Collaborator

Hi Günter,

I see, it seems that AppEngine uses a little bit different approach.

About LoggerFromCustomReceiver

This func (as with the custom receiver registration+config) is meant to construct the logger once and then be used. So if you are going to call it on each message or each request, then it would just lag more than it should and I wouldn't recommend that. Even if you could use some configuration on that.

So, we just have a problem here that currently seelog Context only uses current runtime context, such as func name, file name, etc. and we don't have custom user-made context items. We currently cannot do something like

seelog.TraceCtx(myContextObject, "message", params)

But what I think is that such features could make seelog too complicated, because different logging systems frequently use different approaches and if we try to be fully compatible with them, we will end up as an enormous framework that is not comfortable to use. We even have a wiki page for that: https://github.com/cihub/seelog/wiki/Migration-from-standard-log-package

So what I suggest is to:

  1. Leave the new feature of custom receivers as is. It may be used for custom loggers that use the same ideology as seelog (no custom context in the log calls)
  2. For logging systems that use different approach, don't make seelog more complicated, but just wrap seelog itself.

Comments on point 2:

I feel that if AppEngine logging system needs a context, then it just works differently. All of seelog receivers won't use that context anyway, so I'm not sure using seelog here is a right thing to do. These are two different logging systems with different ideologies that may be just incompatible.

So why not just go and do it a lot easier: make a simple abstraction layer that calls seelog when you want seelog and AppEngine when you want appengine.

Using an intermediate layer is the initial suggestion from the second issue: #38. And seelog already has features that would help you with this approach and they were discussed here: #36

It's going to be like another 30 lines of really simple code, like https://gist.github.com/stephanos/6468975 and this way you just don't need to use incompatible systems as one. You can you them together, side by side. AppEngine where you have its context, seelog -- where you don't.

Could you please think about this approach

@zoechi
Copy link
Author

zoechi commented Sep 24, 2013

Hi goodsign,

thank you for your detailed answer.

Abstract on or the other away is easy done,
but it doesn't solve my problem why I want to use seelog.

I want to profit from the features like custom formatting, information about the method where the log was called, filters, ...

I want to use these things mostly for debugging.
If the lag of initializing a logger would be to heavy I can easily disable seelog in production,
but when I'm trying to localize a specific problem I can activate seelog and profit from its features.
I could even activate it for a specific users if one reports a problem (e.g. using a boolean flag at the user account record).

Even if you think it is not such a good idea I would appreciate it if you could add a method like LoggerFromCustomReceiver but with an additional argument to pass a configuration in addition to the CustomLogger.

What do you think about that?

Indenpendent of that I think CustomLogger improves Seelog a lot.
E.g. if you want to log to syslog, windows eventlog or any other logging backend that is not yet supported directly (I don't know if NetworkWriter doesn't support syslog anyway).

Thanks
Günter

@goodsign
Copy link
Collaborator

Well, let me think about it for a while.

About configuration, basically LoggerFromCustomReceiver is the analog of this config:

<seelog>
    <outputs>
        <custom/>
    </outputs>
</seelog>

where custom points to your custom receiver, that you have just created. I can give you two options to control:

  1. format of the item
  2. type of the logger

so I can LoggerFromCustomReceiverWithTypeFormat (r receiver, Y format, X loggerType) that will be the analog of

<seelog type="X">
    <outputs>
        <custom formatid="form"/>
    </outputs>
    <formats>
        <format id="form" format="Y"/>
    </formats>
</seelog>

But you won't be able to use filters with LoggerFrom* approach anyway. So it feels like a hacky solution that doesn't cover everything. And the whole instantiation stuff is still looking as a hack for me.

Basically, you need to use formatting and context features to find out what func and file you are in, and you need to be able to use these in your message formats. Am I right?

If yes, then I'm going to think for a while to find out what could be a possible solution.

@zoechi
Copy link
Author

zoechi commented Sep 24, 2013

Thank you for your effort.

Yes, I would like to have func and file available for formatting but I'm not sure what you mean with "context features to find out what func and file you are in".
Why would this differ from the default seelog functionality?

If you are talking about the appengine context, then no.
I won't do anything with the appengine context but passing it forward to the appengine method Debugf/Warningf/... inside the CustomReceiver.
If you are talking about something else please explain. I saw, that there is a context in seelog, but I don't know yet what it is for.

@goodsign
Copy link
Collaborator

Hi zoechi,

Yes, I was talking about the seelog context, not the appengine one.

It's really inconvenient that appengine forces apps to work this way. And I cannot think of a solution that I really like in this kind of situation. LoggerFromCustomReceiver won't provide you the possibility to set log level exceptions and other stuff, that must be configured.

The best solution for you that I can currently think of is a possibility to parse config with the same logic as Register, but with the register-like-stuff being a local param when parsing config.

Now you have:

func LoggerFromConfigAsFile(fileName string) (LoggerInterface, error) {...}
func LoggerFromConfigAsBytes(data []byte) (LoggerInterface, error) {...}
func LoggerFromConfigAsString(data string) (LoggerInterface, error) {...}

and you'll additionally get:

type CfgParseParams struct {
    CustomReceivers map[string]CustomReceiver // actual instance here, NOT type (like in RegisterReceiver)
}
func LoggerFromParamConfigAsFile(fileName string, parseParams *CfgParseParams) (LoggerInterface, error) {...}
func LoggerFromParamConfigAsBytes(data []byte, parseParams *CfgParseParams) (LoggerInterface, error) {...}
func LoggerFromParamConfigAsString(data string, parseParams *CfgParseParams) (LoggerInterface, error) {...}

Config parser will check both globally registered types and those that it got in the parse config. The latter will have higher priority.

This way you could use the full functionality of seelog config (because you'll be actually using config) AND you'll be able to register custom loggers per-request.

What do you think?

@zoechi
Copy link
Author

zoechi commented Sep 30, 2013

Hi goodsign,

I think that would perfectly solve my situation.

@goodsign
Copy link
Collaborator

goodsign commented Oct 2, 2013

Hi zoechi!

I've pushed one of the possible solutions, could you please try it?

Now you have LoggerFromParamConfigAsBytes (or AsString/AsFile, like the standard ones) and you can provide a special config param. There you have the CustomReceiverProducers field. It is simply a map that connects custom name and a producer function.

The producer func can contain anything, can use closures, so you can pass your context from the outer scope, etc.

@zoechi
Copy link
Author

zoechi commented Oct 3, 2013

Of couse I will try it. I just have to finish some ongoing refactoring.

@zoechi
Copy link
Author

zoechi commented Oct 5, 2013

Basic logging with custom format works great!
But I have yet to figure out and test filtering.

I sent a pull request for some suggestions.
I couldn't get github to show the diff between gs_issue_39 and my specific branch but only
my branch and master. Maybe you have to manually apply the changes if you like them.

Thank you so much for spending your valuable time.

@goodsign
Copy link
Collaborator

goodsign commented Oct 5, 2013

Hi zoechi,

Yes, typedef for the CustomReceiverProducer seems reasonable. I'll add it to this branch a bit later.

About filtering: basically I didn't change anything, so it should be pretty much the same as with any other (non-custom) receiver. But you could anyway test it and tell me what you think and then I'll proceed with merging this branch.

@zoechi
Copy link
Author

zoechi commented Oct 5, 2013

Exceptions work as well.
Awesome. This improves my development experience dramatically.
Thangs again!

@goodsign
Copy link
Collaborator

goodsign commented Oct 5, 2013

Ok, great! Then I'm leaving current solution as is. I'm going to take a little pause to look at the code once more, etc. But you can already use the logic in your app from this branch, it won't change much.

goodsign added a commit that referenced this pull request Oct 9, 2013
custom dispatcher / AppEngine
@goodsign goodsign merged commit 791566c into master Oct 9, 2013
@goodsign goodsign deleted the gs_issue_39 branch October 9, 2013 19:33
@goodsign
Copy link
Collaborator

goodsign commented Oct 9, 2013

Hi zoechi!

It feels that everything now is implemented. I've merged all the stuff to master. Check the latest master revision or tag 'v2.5'.

Also check the new wiki article:

Custom receivers

There I try to cover all the scenarios that came to my mind and describe them using some code examples. App Engine example is described in the end of the document as an example of the last case scenario.

Thanks for the issues and feel free to tell if anything breaks because of these new changes or if something needs more refinement. BTW if something in wiki seems unclear or broken, you may also create issues on wiki.

@zoechi
Copy link
Author

zoechi commented Oct 17, 2013

Hi goodsing,

I tested master and all works fine.

A nice addition would be a function

func NewCfgParseParams() *seelog.CfgParseParams {
return &seelog.CfgParseParams{
CustomReceiverProducers: make(map[string]seelog.CustomReceiverProducer),
}
}

The wiki page is a good introduction.
I think you don't need fallthroughs the following code if the case statements follow consecutive without empty lines in between.
switch level {
case seelog.TraceLvl:
fallthrough
case seelog.DebugLvl:
ar.l.Debug(context.Func(), message)
case seelog.InfoLvl:
ar.l.Info(context.Func(), message)
case seelog.WarnLvl:
fallthrough
case seelog.ErrorLvl:
fallthrough
case seelog.CriticalLvl:
ar.l.Error(context.Func(), message)
}
return nil
}

Thanks again for the good work!
This is a huge improvement for my GAE development experience.

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

2 participants