Add Docker ID of containers to logs #2399
Conversation
agent/engine/docker_task_engine.go
Outdated
| @@ -1100,7 +1100,7 @@ func getFirelensLogConfig(task *apitask.Task, container *apicontainer.Container, | |||
| } | |||
|
|
|||
| func (engine *DockerTaskEngine) startContainer(task *apitask.Task, container *apicontainer.Container) dockerapi.DockerContainerMetadata { | |||
| seelog.Infof("Task engine [%s]: starting container: %s", task.Arn, container.Name) | |||
| seelog.Infof("Task engine [%s]: starting container: %s (Docker ID: %s)", task.Arn, container.Name, container.RuntimeID) | |||
There was a problem hiding this comment.
how about calling it Runtime ID?
There was a problem hiding this comment.
How about Container ID? Doesnt docker ps call it Container ID?
There was a problem hiding this comment.
how about calling it Runtime ID?
Should be a quick find/replace if you agree with either sharanyad or sparrc. My vote’s for “Runtime ID” (since it’s container.RuntimeID)
There was a problem hiding this comment.
but this is an external-facing log message. Docker doesnt externally call it "Runtime ID" anywhere. For example googling "docker runtime id" produces cryptic results. Googling "docker container id" produces exactly what we're talking about.
There was a problem hiding this comment.
In ECS, we have been calling it Runtime ID - https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_Container.html#ECS-Type-Container-runtimeId
There was a problem hiding this comment.
Thanks for the suggestions. Changed it to "Runtime ID" in order to be consistent with ECS public documentation.
ubhattacharjya
force-pushed
the
RuntimeID
branch
from
2bc9e3f
to
f8582f9
Compare
| @@ -1392,11 +1392,11 @@ func (engine *DockerTaskEngine) applyContainerState(task *apitask.Task, containe | |||
| } | |||
| metadata := transitionFunction(task, container) | |||
| if metadata.Error != nil { | |||
| seelog.Infof("Task engine [%s]: error transitioning container [%s] to [%s]: %v", | |||
| task.Arn, container.Name, nextState.String(), metadata.Error) | |||
| seelog.Infof("Task engine [%s]: error transitioning container [%s (Runtime ID: %s)] to [%s]: %v", | |||
There was a problem hiding this comment.
if there is error in transitioning, it could also have empty container ID..but don't want to add a logic to check states and log accordingly. this is just to keep in mind ID can be empty
There was a problem hiding this comment.
There can be empty runtime ID. But did not add any logic for that so that the code does not become cumbersome.
| @@ -1001,8 +1001,8 @@ func (mtask *managedTask) containerNextState(container *apicontainer.Container) | |||
| } | |||
| if blocked, err := dependencygraph.DependenciesAreResolved(container, mtask.Containers, | |||
| mtask.Task.GetExecutionCredentialsID(), mtask.credentialsManager, mtask.GetResources()); err != nil { | |||
| seelog.Debugf("Managed task [%s]: can't apply state to container [%s] yet due to unresolved dependencies: %v", | |||
| mtask.Arn, container.Name, err) | |||
| seelog.Debugf("Managed task [%s]: can't apply state to container [%s (Runtime ID: %s)] yet due to unresolved dependencies: %v", | |||
There was a problem hiding this comment.
same, can be empty runtime ID?
There was a problem hiding this comment.
There can be empty runtime ID. But did not add any logic for that so that the code does not become cumbersome.
ubhattacharjya
force-pushed
the
RuntimeID
branch
from
f8582f9
to
04f17ce
Compare
ubhattacharjya
force-pushed
the
RuntimeID
branch
from
04f17ce
to
d6f14cc
Compare
Summary
This PR adds container runtime ID to the agent log lines which do not include them for better debugging
Implementation details
Testing
Tested that the log lines pop up with docker id in agent logs
New tests cover the changes:
Description for the changelog
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.