-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Add new Logging driver "fluentd" #12876
Conversation
cdf1f68
to
05df86d
Compare
@tagomoris Overall looks cool. But I'd be much more eager to merge if for msgpack serialization were used something simpler then |
Hmm, I have 2 options:
Using JSON is much more simpler and easy to implement, but it requires more CPU usage if container generates large amount of logs. |
@tagomoris what about Basically |
@LK4D4 make sense. |
Now I'm going to rewrite this patch with JSON serialization, without |
@tagomoris So, whole problem, that we don't want to expose |
There are no problem to send TCP msgpack serialized data, and to add methods like Now I'm concerning about serializer, especially for msgpack. I think that it's better to implement this feature based on JSON serialization at first, because JSON serializer is already used by |
@tagomoris It is pretty easy to generate serializer if we know structure(and we know), and it will be superfast. Makes sense to make logs as fast as we can, it is most loaded part of docker. And generating serializer is very cheap: https://github.com/tinylib/msgp |
@LK4D4 Ah, i got you mentioned finally! |
05df86d
to
8726745
Compare
Updated patch w/ fluent-logger-golang Now fluent-logger-golang has also @LK4D4 Could you review once more? (only dependencies are changed) |
Would it be possible to also include the container name (not just container id) in the tag? |
|
||
clone git github.com/fluent/fluent-logger-golang v0.6.0 | ||
|
||
# get Go tip's archive/tar, for xattr support and improved performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed this stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, i got a mistake on rebase operation of this file. I'll rebase this again and push it.
@fw42 yes, tag can contain any strings, including container name. But too long/complex tags are not good for users in general. |
8726745
to
ec5ba89
Compare
Hmm you mean the application itself should include the container name in it's own logs (in every single line)? That seems like an anti-pattern to me. I don't want my application to (have to) know that it is even running inside a container. |
@tagomoris: To clarify the use-case, I think it would be nice to be able to use different fluentd output plugins based on the container name. If the container name were part of the fluentd tag, that would be pretty easy. Do you have another idea? |
No. Logging driver get container name and id when initialized, like journald logger. I pushed a commit to do so.
Hmm, it seems true for me too. FYI: There're some plugins like |
It will be configurable with |
@tagomoris oh, almost forgot; there's another PR that implements changing the "tag" (to use, e.g. the container-name) for the syslog driver. It hasn't been approved yet, and needs a rebase (because the You can find that PR here; #12668. I think it would make sense to make sure both drivers use the same approach. (Not taking a position here which approach that would be) |
|
||
# docker run -it -p 24224:24224 -v /path/to/conf/test.conf:/fluentd/etc -e FLUENTD_CONF=test.conf fluent/fluentd:latest | ||
|
||
Then, start any containers you want, with `fluentd` logging driver: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the comma after Then?
Looks better! |
Add a commit and pushed. |
Thanks @tagomoris @LK4D4 et al. Maybe it's your turn to do the docs review. |
@@ -932,6 +932,16 @@ driver to a GELF remote server at `192.168.0.42` on port `12201` | |||
|
|||
The `gelf-tag` option specifies a tag for easy container identification. | |||
|
|||
#### Logging driver: fluentd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has some missing tagging and an awkward construction in the second option.
--> https://gist.github.com/moxiegirl/62fd3d0a52d6fb1d4853
Logging driver: fluentd
Fluentd logging driver for Docker. Writes log messages to fluentd
(forward input). The docker logs
command is not available for this logging driver.
You can use the --log-opt NAME=VALUE
flag to specify these additional Fluentd logging driver options.
fluentd-address
: specifyhost:port
to connect [localhost:24224]fluentd-tag
: specify tag forfluentd
message,
When specifying a fluentd-tag
value, you can use the following markup tags:
{{.ID}}
: short container id (12 characters){{.FullID}}
: full container id{{.Name}}
: container name
For example, to specify both additional options:
docker run --log-driver=fluentd --log-opt fluentd-address=localhost:24224 --log-opt fluentd-tag=docker.{{.Name}}
If container cannot connect to the Fluentd daemon on the specified address, the container stops
immediately.
@tagomoris Apologies for stepping on your PR. I wanted to test the new organization and I knew your PR was in docs/review and almost ready to merge. |
Signed-off-by: TAGOMORI Satoshi <[email protected]>
Signed-off-by: TAGOMORI Satoshi <[email protected]>
779dab9
to
78f4a9d
Compare
@moxiegirl @LK4D4 rebased again, on master branch, which already has document file for this driver. |
Thanks @tagomoris for being patient. LGTM |
@LK4D4 ready for merge at will |
Add new Logging driver "fluentd"
@LK4D4 ty. @tagomoris Thank you for this contribution. We really appreciate your work. |
👍 |
1 similar comment
👍 |
Yes, merged! Thanks so much @tagomoris sorry this took so long |
Great work @tagomoris (moris-san) 💯 |
Thank you so much! I'm very looking forward the release of next version! |
Great job @tagomoris and Docker team! 👍 |
Thanks @tagomoris 😄 we've been looking forward to this! |
- Add fluentd logging driver to zsh completion moby#12876 - Add inspect --type flag to zsh completion moby#13187 - Respect -H option in zsh completion moby#13195 - Fix number of argument limit for pause and unpause in zsh completion Signed-off-by: Steve Durrheimer <[email protected]>
- Add fluentd logging driver to zsh completion moby#12876 - Add inspect --type flag to zsh completion moby#13187 - Respect -H option in zsh completion moby#13195 - Fix number of argument limit for pause and unpause in zsh completion Signed-off-by: Steve Durrheimer <[email protected]>
- Add fluentd logging driver to zsh completion moby#12876 - Add inspect --type flag to zsh completion moby#13187 - Respect -H option in zsh completion moby#13195 - Fix number of argument limit for pause and unpause in zsh completion Signed-off-by: Steve Durrheimer <[email protected]>
This patch provides
fluentd
logging plugin for docker, which is mentioned at #12540.How to confirm behavior of this patch/new logging driver.
make BINDDIR=. binary
docker -dD
docker exec -it kickass_heisenberg /bin/bash
docker run --log-driver=fluentd hello-world