feat: add log format annotation and helm value #4620

naseemkullah · 2020-06-17T04:03:45Z

Json log formatting has been added via linkerd/linkerd2-proxy#500
but wiring the option through as an annotation/helm value is still
necessary.

This PR adds the annotation and helm value to configure log format.

Closes #2491

Signed-off-by: Naseem naseem@transit.app

zaharidichev

@naseemkullah Thanks! I see this is still a draft, but just as a note, I assume you are going to make it so LINKERD2_PROXY_LOG_FORMAT on the proxy is actually set. Also, we would ideally want a default value for that and an entry in values.yaml so we do not get a null in the config map

olix0r · 2020-06-18T20:26:53Z

@naseemkullah I think initially we should leave this out of the CLI flags and only support this via Helm & workload annotation. We may consider adding it there later, but for now I think we'd prefer to minimize configuration CLI code.

naseemkullah · 2020-06-19T02:11:26Z

@naseemkullah I think initially we should leave this out of the CLI flags and only support this via Helm & workload annotation. We may consider adding it there later, but for now I think we'd prefer to minimize configuration CLI code.

Sure, np! cli flag removed @olix0r ✅ ...ooc are there other options that are configurable via env var/helm value and/or annotation but not cli flag?

zaharidichev · 2020-06-19T07:05:37Z

@olix0r this sounds good to me. i wonder however, instead of leaving this out of the CLI (much like dst get networks), would it make sense to just put it as a hidden flag?

zaharidichev · 2020-06-19T12:33:54Z

@naseemkullah to answer your question, yes there is for example destinationGetNetworks

alpeb

Thanks @naseemkullah !

A LogFormat entry is missing in the pb.Proxy struct returned in install.go's proxyConfig() that allows picking the default value from values.yaml when installing through the CLI.

Also it appears you didn't push your changes to config.proto as it's missing LogFormat. By looking at config.pb.go it seems you declared a new protobuf struct to house that, but I think a bare string is enough (I don't understand why we did that with LogLevel before 🤷‍♂️ )

charts/linkerd2/README.md

cli/cmd/doc.go

proto/config/config.proto

alpeb · 2020-06-23T11:02:22Z

@naseemkullah one more thing: charts/linkerd2/templates/_config.tpl needs logFormat to be added as well 😬

alpeb · 2020-06-23T16:00:24Z

... aaaand also missing in install.go's buildValuesWithoutIdentity() function. I'll have to admit having to add an entry in 20 different places is not very dev friendly!

alpeb · 2020-06-23T16:19:05Z

@naseemkullah I also realized the support for the webhook injector is missing. That should live under the injectPodSpec() function in pkg/inject/inject.go. And you can add the annotation into pkg/inject/inject_test.go so the unit test checks the mechanism is working.

naseemkullah · 2020-06-23T16:57:21Z

... aaaand also missing in install.go's buildValuesWithoutIdentity() function. I'll have to admit having to add an entry in 20 different places is not very dev friendly

True!

edit: @alpeb I see no mention of proxy log level which I'm using as a reference in buildValuesWithoutIdentity(), is it normal that we will have mention of log format but not log level within it?

alpeb · 2020-06-24T21:49:20Z

I see no mention of proxy log level which I'm using as a reference in buildValuesWithoutIdentity(), is it normal that we will have mention of log format but not log level within it?

It's here

naseemkullah · 2020-06-24T23:31:53Z

I see no mention of proxy log level which I'm using as a reference in buildValuesWithoutIdentity(), is it normal that we will have mention of log format but not log level within it?

It's here

Doh! Thanks @alpeb

naseemkullah · 2020-06-25T02:37:35Z

Can someone please help me get tests to pass?

panic: mismatching message name: got linkerd2.config.Install, want linkerd2.config.LogFormat

goroutine 1 [running]:
google.golang.org/protobuf/internal/impl.legacyLoadMessageDesc(0x29d35a0, 0x25ac340, 0x26d738b, 0x19, 0x0, 0x0)
        /Users/naseemullah/go/pkg/mod/google.golang.org/protobuf@v1.24.0/internal/impl/legacy_message.go:136 +0x882
google.golang.org/protobuf/internal/impl.legacyLoadMessageInfo(0x29d35a0, 0x25ac340, 0x26d738b, 0x19, 0xc000120eb0)
        /Users/naseemullah/go/pkg/mod/google.golang.org/protobuf@v1.24.0/internal/impl/legacy_message.go:48 +0xbd
google.golang.org/protobuf/internal/impl.Export.LegacyMessageTypeOf(0x298d1c0, 0x0, 0x26d738b, 0x19, 0x25ac440, 0xc0001717c0)
        /Users/naseemullah/go/pkg/mod/google.golang.org/protobuf@v1.24.0/internal/impl/legacy_export.go:33 +0xa5
github.com/golang/protobuf/proto.RegisterType(0x298d1c0, 0x0, 0x26d738b, 0x19)
        /Users/naseemullah/go/pkg/mod/github.com/golang/protobuf@v1.4.2/proto/registry.go:186 +0x4d
github.com/linkerd/linkerd2/controller/gen/config.init.0()
        /Users/naseemullah/repos/github.com/linkerd/linkerd2/controller/gen/config/config.pb.go:825 +0x21b
FAIL    github.com/linkerd/linkerd2/cli/cmd     0.023s
FAIL

alpeb · 2020-06-25T12:51:04Z

@naseemkullah it seems there are some issues with the latest protobuf lib update that happened in the main branch. I'll give that a look and let you know.

Regenerated protobuf files, using version 1.4.2 that was upgraded from 1.3.2 with the proxy-api update in #4614. As of v1.4 protobuf messages are disallowed to be copied (because they hold a mutex), so whenever a message is passed to or returned from a function we need to use a pointer. This affects _mostly_ test files. This is required to unblock #4620 which is adding a field to the config protobuf.

alpeb · 2020-06-26T15:14:33Z

@naseemkullah you may now merge the changes from the main branch one more time and run bin/protoc-go.sh to include your changes.

Json log formatting has been added via linkerd/linkerd2-proxy#500 but wiring the option through as an annotation/helm value is still necessary. This PR adds the annotation and helm value to configure log format. Closes linkerd#2491 Signed-off-by: Naseem <naseem@transit.app>

Signed-off-by: Naseem <naseem@transit.app>

alpeb

Thanks @naseemkullah , this looks good to me!

naseemkullah · 2020-06-30T13:02:23Z

Thanks for your help as always @alpeb!

Pothulapati

LGTM!

Tested overriding both at namespace and pod level

naseemkullah mentioned this pull request Jun 17, 2020

Add option for logging in json format #2491

Closed

naseemkullah force-pushed the log-format branch 2 times, most recently from 4434296 to cf5d490 Compare June 17, 2020 04:33

zaharidichev reviewed Jun 17, 2020

View reviewed changes

naseemkullah force-pushed the log-format branch 7 times, most recently from 487e014 to c887835 Compare June 18, 2020 03:23

naseemkullah marked this pull request as ready for review June 18, 2020 03:32

naseemkullah requested review from olix0r and a team as code owners June 18, 2020 03:32

naseemkullah requested a review from zaharidichev June 18, 2020 03:32

naseemkullah changed the title ~~wip: add log format annotation and cli flag~~ feat: add log format annotation and cli flag Jun 18, 2020

naseemkullah force-pushed the log-format branch from c887835 to 460d00b Compare June 19, 2020 01:04

naseemkullah changed the title ~~feat: add log format annotation and cli flag~~ feat: add log format annotation and helm value Jun 19, 2020

naseemkullah force-pushed the log-format branch 3 times, most recently from f948a64 to e08a5f3 Compare June 19, 2020 02:02

naseemkullah force-pushed the log-format branch from 3e9df85 to e08a5f3 Compare June 19, 2020 11:54

alpeb reviewed Jun 22, 2020

View reviewed changes

charts/linkerd2/README.md Outdated Show resolved Hide resolved

cli/cmd/doc.go Outdated Show resolved Hide resolved

proto/config/config.proto Outdated Show resolved Hide resolved

naseemkullah force-pushed the log-format branch from 6dbdba2 to f8a1a08 Compare June 23, 2020 02:36

naseemkullah requested a review from alpeb June 23, 2020 02:43

olix0r changed the base branch from master to main June 24, 2020 18:37

naseemkullah force-pushed the log-format branch from 8a0d675 to f2a0a38 Compare June 24, 2020 23:31

alpeb mentioned this pull request Jun 25, 2020

Upgrade generated protobuf files to v1.4.2 #4673

Merged

naseemkullah force-pushed the log-format branch 2 times, most recently from 4c4368a to c4213d2 Compare June 27, 2020 00:29

update protobuf dependencies

915c962

Signed-off-by: Naseem <naseem@transit.app>

naseemkullah force-pushed the log-format branch from c4213d2 to 915c962 Compare June 27, 2020 00:34

alpeb approved these changes Jun 30, 2020

View reviewed changes

Pothulapati approved these changes Jul 2, 2020

View reviewed changes

alpeb merged commit 361d35b into linkerd:main Jul 2, 2020

alpeb mentioned this pull request Jul 2, 2020

Release edge-20.7.1 #4698

Merged

ArthurSens mentioned this pull request Jul 4, 2020

Enable overriding timeout for DNS canonization through annotation. #4718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add log format annotation and helm value #4620

feat: add log format annotation and helm value #4620

naseemkullah commented Jun 17, 2020 •

edited

Loading

zaharidichev left a comment

olix0r commented Jun 18, 2020 •

edited

Loading

naseemkullah commented Jun 19, 2020 •

edited

Loading

zaharidichev commented Jun 19, 2020

zaharidichev commented Jun 19, 2020

alpeb left a comment

alpeb commented Jun 23, 2020

alpeb commented Jun 23, 2020

alpeb commented Jun 23, 2020

naseemkullah commented Jun 23, 2020 •

edited

Loading

alpeb commented Jun 24, 2020

naseemkullah commented Jun 24, 2020

naseemkullah commented Jun 25, 2020

alpeb commented Jun 25, 2020

alpeb commented Jun 26, 2020

alpeb left a comment

naseemkullah commented Jun 30, 2020

Pothulapati left a comment

feat: add log format annotation and helm value #4620

feat: add log format annotation and helm value #4620

Conversation

naseemkullah commented Jun 17, 2020 • edited Loading

zaharidichev left a comment

Choose a reason for hiding this comment

olix0r commented Jun 18, 2020 • edited Loading

naseemkullah commented Jun 19, 2020 • edited Loading

zaharidichev commented Jun 19, 2020

zaharidichev commented Jun 19, 2020

alpeb left a comment

Choose a reason for hiding this comment

alpeb commented Jun 23, 2020

alpeb commented Jun 23, 2020

alpeb commented Jun 23, 2020

naseemkullah commented Jun 23, 2020 • edited Loading

alpeb commented Jun 24, 2020

naseemkullah commented Jun 24, 2020

naseemkullah commented Jun 25, 2020

alpeb commented Jun 25, 2020

alpeb commented Jun 26, 2020

alpeb left a comment

Choose a reason for hiding this comment

naseemkullah commented Jun 30, 2020

Pothulapati left a comment

Choose a reason for hiding this comment

naseemkullah commented Jun 17, 2020 •

edited

Loading

olix0r commented Jun 18, 2020 •

edited

Loading

naseemkullah commented Jun 19, 2020 •

edited

Loading

naseemkullah commented Jun 23, 2020 •

edited

Loading