Move pkg/jsonlog to be a subpackage of the single consumer by dnephin · Pull Request #34946 · moby/moby

dnephin · 2017-09-22T20:25:24Z

Remove some unused code
Un-export some code that shouldn't be exported
Cleanup test cases
Move a constant to a more appropriate module
Move the pkg/jsonlog to a subpackage of jsonfilelog

Part of Find a place for /pkg #32989

cpuguy83

LGTM

cpuguy83 · 2017-09-23T12:54:49Z

It would be nice to reconcile this with ffjson.

dnephin · 2017-09-23T15:16:36Z

It would be nice to reconcile this with ffjson.

I agree, but I think we need to get it setup with a go generate, and a check in hack/validate to make sure generation is done correctly, and a benchmark to compare against encoding/json.

unclejack · 2017-09-24T08:41:14Z

@dnephin: Can you provide benchmark numbers for memory allocations done before and after these changes, please?

vdemeester · 2017-09-25T07:49:06Z

pkg/jsonlog/jsonlogbytes.go

We are using that one on the client side right ? This might break a bunch of user of this pkg right ?

This struct is not used by the cli. The cli is only using a constant, and is made forward compatible here: docker/cli#551

It might break users of this pkg, but that is unfortunately necessary. These packages were not maintained properly and developed really bad interfaces which need to be fixed. We're going to be breaking everyone with #32989 anyway, and I think this can be seen as cleanup work necessary for #32989.

The godoc (https://godoc.org/github.com/moby/moby/pkg/jsonlog) doesn't show any consumers of this package. I'm not sure how reliable that is, but it seems to work for other packages.

dnephin · 2017-09-25T18:58:16Z

Looking at this again, I don't think JSONLog.MarshalJSON() is actually used anywhere. It seems that object is only ever used to unmarshal JSON.

Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-09-25T20:09:41Z

Can you provide benchmark numbers for memory allocations done before and after these changes

I found some benchmarks in daemon/logger/jsonfilelog and fixed them up to work, then backported them to master. Here's the output.

# Branch
go test -v -bench=. -run TestNothing -benchmem ./daemon/logger/jsonfilelog/
BenchmarkJSONFileLoggerLog-8             1000000              1007 ns/op    172.65 MB/s         121 B/op          1 allocs/op
BenchmarkJSONFileLoggerReadLogs-8        1000000              1243 ns/op    139.98 MB/s         356 B/op          6 allocs/op
PASS
ok      github.com/docker/docker/daemon/logger/jsonfilelog      2.284s

# Master
BenchmarkJSONFileLoggerLog-8             2000000               990 ns/op    175.61 MB/s         122 B/op          1 allocs/op
BenchmarkJSONFileLoggerReadLogs-8        1000000              1137 ns/op    153.01 MB/s         260 B/op          6 allocs/op
PASS
ok      github.com/docker/docker/daemon/logger/jsonfilelog      4.102s

This is the first go benchmark I've written, so let me know if you see anything that looks off. There were some logical errors in what exited before (wrong number of bytes because 1) it was using a different struct to calculate bytes, and 2) it was doing extra iterations not accounted for by b.N)

dnephin · 2017-09-25T20:11:46Z

I removed some more unused code JSONLog.MarshalJSON() was completely unused, which allowed me to remove the duplcate ffjson function. I moved the whole package to under jsonfilelog since this is really just an implementation detail of that logger.

vdemeester

LGTM 🐸

Fixes moby#19803 Updated the json-logger to utilize the common log option 'tag' that can define container/image information to include as part of logging. When the 'tag' log option is not included, there is no change to the log content via the json-logger. When the 'tag' log option is included, the tag will be parsed as a template and the result will be stored within each log entry as the attribute 'tag'. Update: Removing test added to integration_cli as those have been deprecated. Update: Using proper test calls (require and assert) in jsonfilelog_test.go based on review. Update: Added new unit test configs for logs with tag. Updated unit test error checking. Update: Cleanup check in jsonlogbytes_test.go to match pending changes in PR moby#34946. Update: Merging to correct conflicts from PR moby#34946. Signed-off-by: bonczj <[email protected]>

unclejack · 2017-09-27T09:33:29Z

@dnephin: Thank you. I'm worried about regressions related to memory allocations in this code. We've had some unpleasant issues related to this in the past. It's good to see allocations remain pretty much the same.

Fixes moby#19803 Updated the json-logger to utilize the common log option 'tag' that can define container/image information to include as part of logging. When the 'tag' log option is not included, there is no change to the log content via the json-logger. When the 'tag' log option is included, the tag will be parsed as a template and the result will be stored within each log entry as the attribute 'tag'. Update: Removing test added to integration_cli as those have been deprecated. Update: Using proper test calls (require and assert) in jsonfilelog_test.go based on review. Update: Added new unit test configs for logs with tag. Updated unit test error checking. Update: Cleanup check in jsonlogbytes_test.go to match pending changes in PR moby#34946. Update: Merging to correct conflicts from PR moby#34946. Signed-off-by: bonczj <[email protected]>

dnephin requested a review from vdemeester as a code owner September 22, 2017 20:25

GordonTheTurtle added the status/0-triage label Sep 22, 2017

dnephin requested a review from cpuguy83 September 22, 2017 20:25

dnephin mentioned this pull request Sep 22, 2017

Added tag log option to json-logger #34248

Closed

dnephin added status/2-code-review rebuild/janky and removed status/0-triage labels Sep 22, 2017

GordonTheTurtle removed the rebuild/janky label Sep 22, 2017

dnephin mentioned this pull request Sep 22, 2017

Use a local constant docker/cli#551

Merged

dnephin force-pushed the fix-jsonlog branch from 3619bfc to face2f7 Compare September 22, 2017 22:11

aaronlehmann approved these changes Sep 23, 2017

View reviewed changes

cpuguy83 approved these changes Sep 23, 2017

View reviewed changes

cpuguy83 added status/4-merge and removed status/2-code-review labels Sep 23, 2017

dnephin added the rebuild/* label Sep 23, 2017

GordonTheTurtle removed the rebuild/* label Sep 23, 2017

vdemeester reviewed Sep 25, 2017

View reviewed changes

dnephin added 6 commits September 25, 2017 16:07

Move RFC3339NanoFixed to a more appropriate package.

27cfa68

Signed-off-by: Daniel Nephin <[email protected]>

Unexport FastTimeMarshalJSON

7de92de

Signed-off-by: Daniel Nephin <[email protected]>

Remove unused Format

638d4cc

Signed-off-by: Daniel Nephin <[email protected]>

Remove unused JSONLog marshaling

231c5cb

Signed-off-by: Daniel Nephin <[email protected]>

Fix benchmarks and remove more unnecessary code.

a06ad27

Signed-off-by: Daniel Nephin <[email protected]>

Move jsonlog to a subpackage of jsonfilelog

035604c

Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the fix-jsonlog branch from face2f7 to 035604c Compare September 25, 2017 20:07

dnephin changed the title ~~Cleanup pkg/jsonlog~~ Move pkg/jsonlog to be a subpackage of the single consumer Sep 25, 2017

yongtang added rebuild/powerpc labels Sep 26, 2017

GordonTheTurtle removed rebuild/powerpc labels Sep 26, 2017

vdemeester added the rebuild/z label Sep 26, 2017

GordonTheTurtle removed the rebuild/z label Sep 26, 2017

vdemeester approved these changes Sep 26, 2017

View reviewed changes

vdemeester merged commit 7d47823 into moby:master Sep 26, 2017

dnephin deleted the fix-jsonlog branch September 26, 2017 13:59

thaJeztah mentioned this pull request Nov 11, 2017

Bump docker/docker dep kubernetes/kubernetes#55481

Closed

thaJeztah mentioned this pull request Jan 9, 2025

Find a place for /pkg #32989

Open

79 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pkg/jsonlog to be a subpackage of the single consumer#34946

Move pkg/jsonlog to be a subpackage of the single consumer#34946
vdemeester merged 6 commits intomoby:masterfrom
dnephin:fix-jsonlog

dnephin commented Sep 22, 2017 •

edited by thaJeztah

Loading

Uh oh!

cpuguy83 left a comment

Uh oh!

cpuguy83 commented Sep 23, 2017

Uh oh!

dnephin commented Sep 23, 2017

Uh oh!

unclejack commented Sep 24, 2017

Uh oh!

vdemeester Sep 25, 2017

Uh oh!

dnephin Sep 25, 2017

Uh oh!

dnephin commented Sep 25, 2017 •

edited

Loading

Uh oh!

dnephin commented Sep 25, 2017

Uh oh!

dnephin commented Sep 25, 2017

Uh oh!

vdemeester left a comment

Uh oh!

unclejack commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

dnephin commented Sep 22, 2017 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Sep 23, 2017

Uh oh!

dnephin commented Sep 23, 2017

Uh oh!

unclejack commented Sep 24, 2017

Uh oh!

vdemeester Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Sep 25, 2017

Uh oh!

dnephin commented Sep 25, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

unclejack commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnephin commented Sep 22, 2017 •

edited by thaJeztah

Loading

dnephin commented Sep 25, 2017 •

edited

Loading