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

fluent-cat --format none added #606

Merged
merged 1 commit into from
Jun 1, 2015

Conversation

kawanet
Copy link
Contributor

@kawanet kawanet commented May 29, 2015

Input plugins accept format none since #182 but fluent-cat doesn't yet. The tiny patch allows fluent-cat accept it.

echo "a plain text message" | fluent-cat --none debug
echo "another plain text message" | fluent-cat --format none --message-key message debug

The structured raw logs comes here again! I rarely agree for fluent-cat to accept rest of all formats as it should be kept simple and light weighted as it were. I guess, however, that the plain text format is enough convenient for plain simple usages expected for fluent-cat.

The optional --message-key parameter could be removed to keep it simple.

It seems that -f message or -f plain could be more clear rather than -f none at this usage above, by the way. It should follow the vocabulary at format parameter at input plugins.

@sonots
Copy link
Member

sonots commented May 30, 2015

👍 LGTM

@repeatedly
Copy link
Member

If we go with --none, next patch becomes --csv?
So I think --format none is better for the future.

In the first place, --msgpack / --json options are bad for me...

@kawanet
Copy link
Contributor Author

kawanet commented May 30, 2015

Yay! Thank you both for your feedbacks!

The primary objective at the PR is that --format option accepts none value. The other options of both --none and --message-key are not necessary for me. I could agree to remove both, at first.

Usage: fluent-cat [options] <tag>
    -p, --port PORT                  fluent tcp port (default: 24224)
    -h, --host HOST                  fluent host (default: 127.0.0.1)
    -u, --unix                       use unix socket instead of tcp
    -s, --socket PATH                unix socket path (default: /var/run/fluent/fluent.sock)
    -f, --format FORMAT              input format (default: json)
        --json                       same as: -f json
        --msgpack                    same as: -f msgpack
        --none                       same as: -f none
        --message-key KEY            key field for none format (default: message)

(1) adding --csv in the future?

On a care for further extension to accept the other formats like --csv in the future, I don't think they SHOULD NOT come down to fluent-cat. Once fluent-cat accepts --format none, users could transform it via fluent-plugin-parser, etc. on a later stage of fluentd. I prefer fluent-cat stay simple but not fat.

(2) removing --none?

The only value of the independent --none option is to give the same manner of the existing --msgpack and --json options as you mentioned. To be honest, I have had a little worry about the independent --none which could misguide users to expect a wrong feature something like /dev/null blackhole. Instead of the spell of --none, --plain could be another option to make it clear. However, plain is not a vocabulary of input plugins, but none is. The help message above could guide users to understand it rather than misunderstand it. This means --none is only worth with the help message ;)

Conclusion on my view:

  • --format none gives the most simple way to store logs to fluentd because none is the most plain and general format.
  • The alias of --none is almost acceptable because (1) --csv should not come in the future, (2) the line of --none at the help message could show the same manner of the existing options of --json and --msgpack.
  • Removing --json and --msgpack is not a business of the PR. The other PR may manage it. I don't need to remove them though.
  • --message-key could be removed for fluent-cat to keep itself most simple in design. Any opinions on it?

@repeatedly
Copy link
Member

I don't have strong counter opinions.
fluent-cat is testing tools and shorter options are useful for beginners.

If there is no concerns, I will merge this PR later.

repeatedly added a commit that referenced this pull request Jun 1, 2015
@repeatedly repeatedly merged commit 1598cac into fluent:master Jun 1, 2015
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.

3 participants