Skip to content

Change since parameter in LogContainerCmd to String #1111

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

Merged
merged 1 commit into from
Oct 15, 2018

Conversation

caspergreen
Copy link
Contributor

@caspergreen caspergreen commented Sep 28, 2018

The docker logs-command takes on a variety of different formats for the since parameter.

This PR allows for more formats when using since parameter for the LogContainerCmd.


This change is Reviewable

@KostyaSha KostyaSha added this to the 3.1.0-rc-6 milestone Oct 15, 2018
@KostyaSha KostyaSha merged commit 80c6213 into docker-java:master Oct 15, 2018
@caspergreen caspergreen deleted the feature/log-cmd-to-string branch October 15, 2018 15:33
@fengxx
Copy link
Contributor

fengxx commented Nov 27, 2018

docker logs --since '2013-01-02T13:23:37' doesn't mean the engine api accepts strings, the docker cli itself converts the string into unix epoch.

docker logs --since "2018-12-03T10:15:30+01:00" 7a72c45e2362
#then check /var/log/messages
time="2018-11-26T22:11:04.498966864-08:00" level=debug msg="Calling GET /v1.30/containers/7a72c45e2362/logs?since=1543828530.000000000&stderr=1&stdout=1&tail=all"

I think we need undo the PR

@KostyaSha
Copy link
Member

ok

@caspergreen
Copy link
Contributor Author

I have a container test setup where I need logs within a fraction of a second. What do you think the best approach for this is?

The API allows for more than just integers ( GET /v1.30/containers/7a72c45e2362/logs?since=1543828530.000000000&stderr=1&stdout=1&tail=all). I don't know if a long would work

@fengxx
Copy link
Contributor

fengxx commented Nov 28, 2018

my 2 cents FYI:
keep existing api unchanged and introduce new one e.g. withNanoSeconds in LogContainerCmd for the nano part and update LogContainerCmdExec to set since using "sinceSeconds.nanoSeconds"

@KostyaSha
Copy link
Member

KostyaSha commented Nov 28, 2018

How docker cli handle this?
Could you reopen PR to see that it's still actual? I revert it for the next discussions. docker-java needs have correct model, but can provide additional things.

@caspergreen
Copy link
Contributor Author

The Docker CLI uses the moby client:
https://github.com/moby/moby/blob/master/client/container_logs.go

Which parses the timestamp before making the call to Docker API:
https://github.com/moby/moby/blob/852542b3976754f62232f1fafca7fd35deeb1da3/api/types/time/timestamp.go#L26

The final format: Sprintf("%d.%09d", t.Unix(), int64(t.Nanosecond()))

As @fengxx suggested, adding a new method would make sense. Maybe name it withSinceNanoSeconds(Integer) so it is clear it isn't related to a potential until query parameter.

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