Log objects directly.

[takawitter]

I made patch to support logging Java objects and some primitives.
This patch enables users to log objects like these:

logger.log("tag", "hello");
logger.log("tag", new Msg("hello", true));

outputs:

2013-09-03T01:06:35+00:00       myapp.tag  "hello"
2013-09-03T01:06:35+00:00       myapp.tag  {"message":"hello","valid":true"}

I also leave log(String, Map<String, Object>) method available.

regards.

[oza]

Thanks for your contribution, @takawitter! I've checked the overview of your commits, and it seems to take a bit of time to review. Could you wait a moment? Thanks, Tsuyoshi

[oza]

@takawitter: I ran mvn test, but org.fluentd.logger.sender.TestRawSocketSender fails unfortunately. Could you fix it at first?

The log is as follows:

Running org.fluentd.logger.sender.TestRawSocketSender
2013/09/05 23:11:36 org.fluentd.logger.sender.RawSocketSender open
fatal: Failed to connect fluentd: localhost/127.0.0.1:25227
2013/09/05 23:11:36 org.fluentd.logger.sender.RawSocketSender open
fatal: Connection will be retried
java.net.ConnectException: Connection refused
        at java.net.PlainSocketImpl.socketConnect(Native Method)
        at java.net.PlainSocketImpl.doConnect(PlainSocketImpl.java:351)
        at java.net.PlainSocketImpl.connectToAddress(PlainSocketImpl.java:213)
        at java.net.PlainSocketImpl.connect(PlainSocketImpl.java:200)
        at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:432)
        at java.net.Socket.connect(Socket.java:529)
        at java.net.Socket.connect(Socket.java:478)
        at org.fluentd.logger.sender.RawSocketSender.connect(RawSocketSender.java:89)
        at org.fluentd.logger.sender.RawSocketSender.open(RawSocketSender.java:77)
        at org.fluentd.logger.sender.RawSocketSender.<init>(RawSocketSender.java:72)
        at org.fluentd.logger.sender.RawSocketSender.<init>(RawSocketSender.java:60)
        at org.fluentd.logger.sender.RawSocketSender.<init>(RawSocketSender.java:56)
        at org.fluentd.logger.sender.TestRawSocketSender.testNormal03(TestRawSocketSender.java:134)

[takawitter]

Thanks for your comment, oza.

The exception you pointed seems to be designed to be thrown in test code.
See the test code of testNormal03 method. There is the comments:

// it should be failed to connect to fluentd

Though the log message said fatal, but no test case failed.
And this behavior has not changed by my commit.

regards.
Takao.

[oza]

Oops, I have a mistake about my local branch management. Thanks for your point, and please wait a moment.

[oza]

The confusing log message at testing is filed as #7. Thank you for reporting.

The review comment is as follows:
could you explain your use case? Other loggers(e.g. fluent-logger-ruby) doesn't support to write raw object itself as records, but support to write only map object. This keeps fluent-logger-*'s design simple. fluent-logger-java is one of that. Your pull request includes big changes to EventTemplate, and it gets more complex than what it is. Therefore, we need a big motivation to support this feature.

Thanks, Tsuyoshi

[oza]

Or, if we can design the feature more simply, I'd like to merge your pull request. Now I'm thinking more simple one.

[takawitter]

Thank you for the consideration.

I investigated fluentd feature, it seems to be designed to accept JSON map (not JSON array or values).
So at least we should limit data variation log method accept to

• Map<String, Object> (Object accepts values, array, Map<String, Object> and POJO)
• POJO

My motivations as follows:
Designing log message structure as class makes coding easy and reduce bug.
Because of lack of map literal in Java, building structured log message is a quite bother.
In addition, accepting POJO, the user of log message class don't need to write key names of map.
And It can reduce misspelling of key names.

regards.
Takao.

[oza]

I apologize for the delay. The use case you pointed out looks good to me. Thank you for the sharing :-)

The review comment against the implementation:
IMHO, it's enough for users to support message-packable events. These include the events as follows:

1. events which is annotated as @Message @Beans.
2. events which registers Template.

IIUC, events described in 1. is the same events the method writeObj can serialize.
Therefore, we should delete the method writeObj and throw MessageTypeException when message-unpackable events are passed to emit method as follows:

        public void write(Packer pk, Event v, boolean required) throws IOException {
            if (v == null) {
                if (required) {
                    throw new MessageTypeException("Attempted to write null");
                }   
                pk.writeNil();
                return;
            }   

            pk.writeArrayBegin(3);
            {   
                Templates.TString.write(pk, v.tag, required);
                Templates.TLong.write(pk, v.timestamp, required);
                if(v.data instanceof Map){
                    writeMap(pk, (Map<?, ?>)v.data, required);
                } else{
                    pk.write(v.data); // We should throw exception if we don't know how to serialize it!
                }   
            }   
            pk.writeArrayEnd();
        }   

What do you think?

[takawitter]

writeObj method writes map value of POJO.
Neither @message nor @beans can write map value, these write array value.
When you execute following code,

    System.out.println(mp.read(mp.write(new Msg("bob", 20))));

you will get the string: ["bob",20], not {"name":"bob","age":20}.
That is because Msgpack serializes only values, though writeObj method
serializes names and values as map.

regards.

[oza]

IMHO, it's enough for users to support message-packable events. These include the events as follows:

1. events which is annotated as @Message, @Beans.
2. events which registers Template
   IIUC, events described in 1. is the same events the method writeObj can serialize.

writeObj method writes map value of POJO.

You're correct and I may confuse you because of my expression of the sentence. I apologize for this. What I'd like to say here is: the events described in 1. is subset of writeObj can serialize. And my thought is that it can be not only enough but also simple.

This is summary of my thought: I agree that fluent-logger-java should accept POJOs themselves. However, when we would like to send them as Map, we should set key explicitly to avoid subtle cases currently. One subtle case is as follows:

    public static class ParentMsg {
        final private String strForTesting = "hoge";

        public ParentMsg() {
        }   
        public String getStrForTesting() {
            return strForTesting;
        }   
    }   

    public static class Msg extends ParentMsg {
        public Msg() {
          super();
        }   
        public Msg(String name) {
            this.name = name;
        }   

        private String name;
    }   

Your proposal sends "strForTesting" => "hoge" as a part of events when Msg objects are serialized. Is it really a part of events you'd like to send?

One alternative to omit Map's key is that your proposal is done in msgpack-java layer, as an extension of annotation - @Beans or @Message. Is it not enough in your use case?

[takawitter]

Your proposal sends "strForTesting" => "hoge" as a part of events when Msg objects are serialized. Is it really a
part of you'd like to send?

Yes. Converting Java Object to map value is what I wanted to do. Not array value.
And it should account all public getter method including inherited methods.
So @beans and @message are not suitable for my perpose.

One alternative to omit Map's key is that your proposal is done in msgpack-java layer, as an extension of
annotation - @beans or @message. Is it not enough in your use case?

Did you mean extending msgpack-java to support map-style serialization such like @message(style=SerializationStyle.map) ?
That is interesting and very effective idea except demerit of appearing msgpack-java to fluent-logger user and impleentation cost :-)
Anyway, I will try to hack msgpack-java side by side with this issue.

thanks.

[takawitter]

I implemented @message(style=SerializationStyle.MAP).
Here is the difference to original msgpack-java:
https://github.com/takawitter/msgpack-java/compare/mapstyle

What do you think of this kind of change?
I think that is not attractive for msgpack-java comitters because supporting map-style serialization
brings new design issue. Map represents names and values, while array represents
values and orders. So when msgpack-java supports map-style serialization, you have to
rethink how msgpack-java serialization would be.

Anyway, I considered about alternative.
That is:
a. make fluent-logger-java accepts Object as logged message and pass it to msgpack directly (same as your suggestion above).
b. and add MapStyleEventTemplate to let user can choose serialization style of Object ( by setting to EventTemplate.INSTANCE variable).
c. and also make msgpack visible (e.g. add FluentLogger.getMsgPack() method).

By these change, users can simply choose object serialization style or customize serialization
using @message annotation or registering templates.

p.s. my motivation substantiate by a. and b. c. is for superior customizability.

best regards.

[takawitter]

I added the commit that realizes above a. and b. But not yet c. Because I
don't have good way to exposure Msgpack instance RawSocketSender has.

best regards.