Skip to content
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

Feature implement json logformat #91608

Merged

Conversation

yuzhiquan
Copy link
Member

@yuzhiquan yuzhiquan commented May 31, 2020

What type of PR is this?

/kind feature

What this PR does / why we need it:
Feature implement json logformat
Part of kubernetes/enhancements#1602 effort

Allow users to change log out of k8s components by passing flag --log-format=json.
Which issue(s) this PR fixes:

Fixes #91490

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none do-not-merge/work-in-progress kind/feature size/L cncf-cla: yes needs-sig needs-priority labels May 31, 2020
@k8s-ci-robot k8s-ci-robot requested review from andrewsykim, cheftako and May 31, 2020
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency area/kubectl sig/api-machinery labels May 31, 2020
@yuzhiquan
Copy link
Member Author

@yuzhiquan yuzhiquan commented May 31, 2020

Still working on unit test and benchmark.
@serathius PLZ have a look.

@k8s-ci-robot k8s-ci-robot added sig/cli sig/cloud-provider sig/cluster-lifecycle sig/instrumentation and removed needs-sig labels May 31, 2020
@serathius
Copy link
Contributor

@serathius serathius commented May 31, 2020

I'm worried about getting approvals for dependencies at this stage. Can you try to avoid adding dependencies in so many directories? would moving json.go to subdirectory help?

@yuzhiquan
Copy link
Member Author

@yuzhiquan yuzhiquan commented May 31, 2020

Yes, i worried about this too.
After run 'go get go.uber.org/zap@v1.10.0 ', many go.sum updated.

@yuzhiquan yuzhiquan force-pushed the feature-implement-json-logformat branch from 3af1002 to 10343f8 Compare Jun 1, 2020
@yuzhiquan
Copy link
Member Author

@yuzhiquan yuzhiquan commented Jun 1, 2020

Move json to staging/src/k8s.io/component-base/logs/json/ subdir, updated less go.sum.

@yuzhiquan yuzhiquan force-pushed the feature-implement-json-logformat branch from 35754ec to 0d36fdc Compare Jun 1, 2020
@yuzhiquan yuzhiquan force-pushed the feature-implement-json-logformat branch from a3c7e17 to e64eadd Compare Jun 10, 2020
@k8s-ci-robot k8s-ci-robot removed the approved label Jun 10, 2020
@yuzhiquan
Copy link
Member Author

@yuzhiquan yuzhiquan commented Jun 10, 2020

@yuzhiquan Please rebase your change on master

#90976 added dependency on k8s.io/component-base to k8s.io/csi-translation-lib and k8s.io/cloud-provider
This causes pull-kubernetes-dependencies to fail due

Your vendored results are different:
diff -Naupr -x BUILD -x 'AUTHORS*' -x 'CONTRIBUTORS*' vendor/k8s.io/cloud-provider/go.sum /home/prow/go/src/k8s.io/kubernetes/_tmp/kube-vendor.wqWKpW/kubernetes/vendor/k8s.io/cloud-provider/go.sum
--- vendor/k8s.io/cloud-provider/go.sum	2020-06-10 09:50:07.148773270 +0000
+++ /home/prow/go/src/k8s.io/kubernetes/_tmp/kube-vendor.wqWKpW/kubernetes/vendor/k8s.io/cloud-provider/go.sum	2020-06-10 09:54:05.227977279 +0000
@@ -200,6 +200,9 @@ github.com/stretchr/testify v1.4.0/go.mo
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
 go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=
 go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
+go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
+go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
+go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
diff -Naupr -x BUILD -x 'AUTHORS*' -x 'CONTRIBUTORS*' vendor/k8s.io/csi-translation-lib/go.sum /home/prow/go/src/k8s.io/kubernetes/_tmp/kube-vendor.wqWKpW/kubernetes/vendor/k8s.io/csi-translation-lib/go.sum
--- vendor/k8s.io/csi-translation-lib/go.sum	2020-06-10 09:50:07.516796768 +0000
+++ /home/prow/go/src/k8s.io/kubernetes/_tmp/kube-vendor.wqWKpW/kubernetes/vendor/k8s.io/csi-translation-lib/go.sum	2020-06-10 09:54:17.912787457 +0000
@@ -183,6 +183,9 @@ github.com/stretchr/testify v1.4.0/go.mo
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
 go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=
 go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
+go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
+go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
+go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
Vendor Verify failed.

That's very helpful, even not notice this.

@serathius
Copy link
Contributor

@serathius serathius commented Jun 10, 2020

/assign @davidz627 @dims
Please approve changes to go.mod

@serathius
Copy link
Contributor

@serathius serathius commented Jun 10, 2020

/retest

1 similar comment
@serathius
Copy link
Contributor

@serathius serathius commented Jun 10, 2020

/retest

@serathius
Copy link
Contributor

@serathius serathius commented Jun 11, 2020

ping @davidz627 @dims

@k8s-ci-robot k8s-ci-robot added the needs-rebase label Jun 13, 2020
@yuzhiquan yuzhiquan force-pushed the feature-implement-json-logformat branch from e64eadd to 03481e1 Compare Jun 14, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase label Jun 14, 2020
@yuzhiquan
Copy link
Member Author

@yuzhiquan yuzhiquan commented Jun 14, 2020

Rebase.
/assign @davidz627 @dims
ping @davidz627 @dims

add unit test case

add benchmark test case

fix staticcheck, and response for review comment

remove unnecessary variable

add test case for non-string field or zap-field, refactor code

update vendor
@yuzhiquan yuzhiquan force-pushed the feature-implement-json-logformat branch from 03481e1 to a0f808f Compare Jun 14, 2020
@serathius
Copy link
Contributor

@serathius serathius commented Jun 15, 2020

pinging other approvers
@saad-ali @msau42
@andrewsykim @lavalamp

@dims
Copy link
Member

@dims dims commented Jun 15, 2020

/approve
/assign @liggitt

@lavalamp
Copy link
Member

@lavalamp lavalamp commented Jun 15, 2020

/approve

for go.sum file

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jun 15, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, lavalamp, mtaufen, serathius, sttts, yuzhiquan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved label Jun 15, 2020
@serathius
Copy link
Contributor

@serathius serathius commented Jun 15, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 15, 2020
@serathius
Copy link
Contributor

@serathius serathius commented Jun 16, 2020

/retest

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Jun 16, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit fd61c31 into kubernetes:master Jun 16, 2020
17 of 18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 16, 2020
@yuzhiquan yuzhiquan deleted the feature-implement-json-logformat branch Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apiserver area/cloudprovider area/dependency area/kubectl cncf-cla: yes kind/feature lgtm ok-to-test priority/important-soon release-note-none sig/api-machinery sig/cli sig/cloud-provider sig/cluster-lifecycle sig/instrumentation size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.