-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
enhancement(docker source): Enrich events with metadata #1149
Conversation
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
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.
I'd like to see the task unififcation change but otherwise this looks great.
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@LucioFranco and what do you think about the above question regarding different metadata? |
@ktff oops, I thought I answered that! I say go with the simple solution of calling inspect when we need it. We should be able to cache the result and it shouldn't be expensive at all. |
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
With the last commit, events will be enriched with one container name, instead of a list of container names. |
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff this looks great! @LucioFranco will be back from vaction next week and we can get this reviewed one last time. |
Noting, just assigning @LucioFranco for review which will happen next week. |
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.
LGTM
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.
@ktff this looks great! I'd like to make some changes to the metadata fields.
- The docker specific metadata fields should not reside in
event/mod.rs
since they are specific to thedocker
source. Can we remove them from there? Specifically, theSTREAM
,IMAGE
,CONTAINER
,CREATED_AT
andNAME
fields. - I find some of the naming ambiguous. Can we rename the fields to:
container_id
,container_name
,container_created_at
,image
, andstream
.
I am updating the docs now to reflect these changes. Once done we should be good to merge.
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
I've updated the documentation to reflect the above changes. Once they're made in the code this should be good to merge. |
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
Closes #1042
There is one open question:
Currently, metadata is coming from two api endpoints,
container list
andcontainer inspect
. They are returning slightly different data:container list
returns UNIX timestamp,container inspect
returns rfc3339 timestampcontainer list
returns list of names,container inspect
returns one nameThis will result in containers having different metadata depending on when was vector started relative to it.
This could be avoided if we only use metadata from
container inspect
endpoint. With the cost of sometimes additional call to docker api, and loss of extra names. As a side effect, event metadata would instead of list of names have one name.