Add support to custom log level through command line flag and environment variable #842

vitorhugoro1 · 2024-05-13T17:49:25Z

Added on InstrumentationOption the option to define log level.
Implemented a custom severity enum, called level.go, with options to set log levels based on text using zap.Level as reference.
Added on CLI the flag -log-level that accept one of Level value, which can be see on level.go ex. otel-go-instrumentation -log-level error, the default value is info.
Added support to OTEL_LOG_LEVEL which implements log.Level to set visibility level through environment variable.
Changed multiples logs level to Debug to improve setup when using in production environment.
Closes Add log verbosity configuration and move the log in controller.go to debug level #691

linux-foundation-easycla · 2024-05-13T17:49:29Z

The committers listed above are authorized under a signed CLA.

✅ login: vitorhugoro1 / name: Vitor Hugo Rodrigues (48844b5, 7c14c42, b667c33, 6d82618, f2da21b, 6828570, cd22ffd, fd41878, 9746e5c, 26c7732, e2f71ef, f4c537b, c1d1cd7)

instrumentation.go

grcevski · 2024-05-14T15:57:16Z

instrumentation_test.go

@@ -172,6 +172,14 @@ func TestWithResourceAttributes(t *testing.T) {
 	})
 }

+func TestWithLogLevel(t *testing.T) {
+	c, err := newInstConfig(context.Background(), []InstrumentationOption{WithLogLevel("error")})


Thanks for adding these tests! Do you mind extending the testing to cover what happens if invalid options are specified or the configuration doesn't contain logging level?

This is related to my earlier comment that parsing of the log level should be done here, so that invalid command line input is detected.

Created the test to cover the validation of a wrong logging level

MrAlias · 2024-05-14T19:05:58Z

instrumentation.go

+
+// WithLogLevel returns an [InstrumentationOption] that will configure
+// an [Instrumentation] with the logger level visibility defined as inputed.
+func WithLogLevel(level string) InstrumentationOption {


Instead of accepting a string with special values, we should introduce a LogLevel type with enum values. We might want to use the OTel levels. For example:

https://github.com/open-telemetry/opentelemetry-go/blob/main/log/severity.go

Do you think we need to create something which uses numbers as reference like the one you linked?

That seems reasonable to me.

We already do something like this for offsetgen:

opentelemetry-go-instrumentation/internal/tools/inspect/cmd/offsetgen/main.go

Line 59 in e85ddd9

flag.IntVar(&verbosity, "v", 0, "log verbosity")

We could provided human readable flags for the CLI though (i.e. --log-level info).

Yeah, I usually prefer something human readable to CLI because is more quick to identify.

@MrAlias I have create a simple Level Enum for logs, what do you think?

@MrAlias I have create a simple Level Enum for logs, what do you think?

This still accepts a string.

Suggested change

func WithLogLevel(level string) InstrumentationOption {

func WithLogLevel(level Level) InstrumentationOption {

@MrAlias This case here expect an string because is the input when use flag or from env, so for properly work is these scenarios this need to be string

The environment variable needs to be parsed before it is passed to this function then.

We need to use syntax here to ensure the best possible chance values passed will be valid. Using an arbitrary string with magic values is not appropriate.

@MrAlias changed WithLogLevel to work on the behaviour you said

RonFed · 2024-05-15T06:17:23Z

@vitorhugoro1 Do you plan on changing the level of the log line I mentioned in the issue?
In addition, we can use debug level in our tests at

opentelemetry-go-instrumentation/.github/workflows/e2e/k8s/sample-job.yml

Line 29 in e85ddd9

command: ["/otel-go-instrumentation", "-global-impl"]

pellared

Shouldn't we support setting via OTEL_LOG_LEVEL env var?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

MrAlias · 2024-05-15T14:30:54Z

Shouldn't we support setting via OTEL_LOG_LEVEL env var?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

That is tracked in #691. It is out of scope for this PR.

pellared · 2024-05-15T15:56:39Z

Shouldn't we support setting via OTEL_LOG_LEVEL env var?
Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

That is tracked in #691. It is out of scope for this PR.

This PR says

Closes #691

vitorhugoro1 · 2024-05-15T17:28:42Z

Shouldn't we support setting via OTEL_LOG_LEVEL env var?
Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

That is tracked in #691. It is out of scope for this PR.

Yeah this PR will implement this change too, I take the approach just with flags because is the way I usually use log levels on container

… level

vitorhugoro1 · 2024-05-15T19:47:04Z

@vitorhugoro1 Do you plan on changing the level of the log line I mentioned in the issue? In addition, we can use debug level in our tests at

opentelemetry-go-instrumentation/.github/workflows/e2e/k8s/sample-job.yml

Line 29 in e85ddd9

command: ["/otel-go-instrumentation", "-global-impl"]

I changed some log level, not just in the line you mentioned but in another which is more suitable to debugging not exactly on a production setup.

Changed the sample job too to use debug level by default

vitorhugoro1 · 2024-05-15T19:48:05Z

Shouldn't we support setting via OTEL_LOG_LEVEL env var?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

Added the supporting via environment with the name you suggest

instrumentation.go

internal/pkg/instrumentation/manager.go

internal/pkg/process/allocate.go

internal/pkg/process/discover.go

instrumentation.go

cli/main.go

vitorhugoro1 · 2024-05-21T13:17:30Z

@pellared @MrAlias @grcevski it's okay now for you guys?

grcevski

LGTM!

edeNFed · 2024-05-27T12:44:48Z

Can we merge this?
@MrAlias @pellared

MrAlias · 2024-05-28T20:25:45Z

internal/pkg/log/level.go

+	// Log Level [dpanic].
+	LevelDPanic Level = "dpanic"


What is dpanic?

Same as here

#842 (comment)

I will remove this cases

I removed these options

MrAlias · 2024-05-28T20:26:12Z

internal/pkg/log/level.go

+	// Log Level [panic].
+	LevelPanic Level = "panic"
+	// Log Level [fatal].
+	LevelFatal Level = "fatal"


What is the difference between panic and fatal? Shouldn't these both be terminations of the application?

These two is to replicate what we see on zapcore.levels which is our base config

I would replicate OTel logging levels if we are going to model this on a preexisting structure.

I will remove these too

I removed these options because they don't seem to make sense for the project

MrAlias · 2024-05-28T20:26:55Z

instrumentation.go

+
+// WithLogLevel returns an [InstrumentationOption] that will configure
+// an [Instrumentation] with the logger level visibility defined as inputed.
+func WithLogLevel(level string) InstrumentationOption {


@MrAlias I have create a simple Level Enum for logs, what do you think?

This still accepts a string.

Suggested change

func WithLogLevel(level string) InstrumentationOption {

func WithLogLevel(level Level) InstrumentationOption {

MrAlias · 2024-05-28T20:27:24Z

internal/pkg/log/level.go

+	"fmt"
+)
+
+type Level string


Missing comment.

Added comment

MrAlias · 2024-05-28T20:28:15Z

internal/pkg/log/level.go

+	// Log Level [debug].
+	LevelDebug Level = "debug"
+	// Log Level [info].
+	LevelInfo Level = "info"
+	// Log Level [warn].
+	LevelWarn Level = "warn"
+	// Log Level [error].
+	LevelError Level = "error"
+	// Log Level [dpanic].
+	LevelDPanic Level = "dpanic"
+	// Log Level [panic].
+	LevelPanic Level = "panic"
+	// Log Level [fatal].


These comments do not explain the levels. Please update so they communicate the logging strata they intend to enable.

Added comments to levels

MrAlias · 2024-05-28T20:28:38Z

internal/pkg/log/level.go

+	"fmt"
+)
+
+type Level string


This needs to be in the exported auto package so users can reference it.

I did not found where I could make this, can you explain to me?

This lives in an internal package currently. It needs to be moved to the user accessible go.opentelemetry.io/auto package instead.

@MrAlias nice, changed to auto package the level

added log level through commnd line flag

7c14c42

vitorhugoro1 requested a review from a team as a code owner May 13, 2024 17:49

Merge branch 'main' into issue-691

b667c33

vitorhugoro1 mentioned this pull request May 13, 2024

Odiglet is emitting too much info logs odigos-io/odigos#1026

Open

vitorhugoro1 and others added 2 commits May 13, 2024 17:03

Merge branch 'main' into issue-691

f2da21b

added changelog entry

48844b5

grcevski reviewed May 14, 2024

View reviewed changes

MrAlias added this to the v0.12.0-alpha milestone May 14, 2024

MrAlias reviewed May 14, 2024

View reviewed changes

pellared reviewed May 15, 2024

View reviewed changes

vitorhugoro1 added 3 commits May 15, 2024 16:40

implemented own level enum and add err when not found the inputed log…

fd41878

… level

changed log visibility to debug on some info logs

26c7732

changed test job to use debug log level

6d82618

vitorhugoro1 requested review from pellared, grcevski and MrAlias May 15, 2024 19:48

vitorhugoro1 changed the title ~~Add log level through command line flag~~ Add support to custom log level through command line flag and environment variable May 15, 2024

running precommit

9746e5c

grcevski reviewed May 16, 2024

View reviewed changes

instrumentation.go Show resolved Hide resolved

internal/pkg/instrumentation/manager.go Outdated Show resolved Hide resolved

internal/pkg/process/allocate.go Outdated Show resolved Hide resolved

internal/pkg/process/discover.go Outdated Show resolved Hide resolved

adjusted log visibility levels to ensure is following the flag

e2f71ef

vitorhugoro1 requested a review from grcevski May 17, 2024 01:51

RonFed reviewed May 17, 2024

View reviewed changes

instrumentation.go Show resolved Hide resolved

cli/main.go Outdated Show resolved Hide resolved

cli/main.go Outdated Show resolved Hide resolved

changed the behaviour on how default value is set

f4c537b

vitorhugoro1 requested a review from RonFed May 17, 2024 13:45

RonFed approved these changes May 17, 2024

View reviewed changes

Merge branch 'main' into issue-691

6828570

grcevski approved these changes May 21, 2024

View reviewed changes

MrAlias requested changes May 28, 2024

View reviewed changes

removed not used levels and add comments to levels

cd22ffd

vitorhugoro1 requested a review from MrAlias May 29, 2024 14:06

changed level to auto package and use Level as parameter on WithLogLevel

c1d1cd7

	func WithLogLevel(level string) InstrumentationOption {
	func WithLogLevel(level Level) InstrumentationOption {

Add support to custom log level through command line flag and environment variable #842

Are you sure you want to change the base?

Add support to custom log level through command line flag and environment variable #842

Conversation

vitorhugoro1 commented May 13, 2024 • edited

linux-foundation-easycla bot commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonFed commented May 15, 2024 • edited

pellared left a comment

Choose a reason for hiding this comment

MrAlias commented May 15, 2024 • edited

pellared commented May 15, 2024

vitorhugoro1 commented May 15, 2024

vitorhugoro1 commented May 15, 2024

vitorhugoro1 commented May 15, 2024

vitorhugoro1 commented May 21, 2024

grcevski left a comment

Choose a reason for hiding this comment

edeNFed commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitorhugoro1 commented May 13, 2024 •

edited

linux-foundation-easycla bot commented May 13, 2024 •

edited

RonFed commented May 15, 2024 •

edited

MrAlias commented May 15, 2024 •

edited