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

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

Merged
merged 17 commits into from
Jul 1, 2024

Conversation

vitorhugoro1
Copy link
Contributor

@vitorhugoro1 vitorhugoro1 commented May 13, 2024

  • 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 LogLevel 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 LogLevel 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

@vitorhugoro1 vitorhugoro1 requested a review from a team May 13, 2024 17:49
Copy link

linux-foundation-easycla bot commented May 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

instrumentation.go Show resolved Hide resolved
instrumentation_test.go Outdated Show resolved Hide resolved
@MrAlias MrAlias added this to the v0.12.0-alpha milestone May 14, 2024
instrumentation.go Outdated Show resolved Hide resolved
@RonFed
Copy link
Contributor

RonFed commented May 15, 2024

@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

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

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MrAlias
Copy link
Contributor

MrAlias commented May 15, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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

@vitorhugoro1
Copy link
Contributor Author

@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

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
Copy link
Contributor Author

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

@vitorhugoro1 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
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
instrumentation.go Show resolved Hide resolved
cli/main.go Outdated Show resolved Hide resolved
cli/main.go Outdated Show resolved Hide resolved
@vitorhugoro1 vitorhugoro1 requested a review from RonFed May 17, 2024 13:45
@vitorhugoro1
Copy link
Contributor Author

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

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@edeNFed
Copy link
Contributor

edeNFed commented May 27, 2024

Can we merge this?
@MrAlias @pellared

internal/pkg/log/level.go Outdated Show resolved Hide resolved
internal/pkg/log/level.go Outdated Show resolved Hide resolved
instrumentation.go Outdated Show resolved Hide resolved
internal/pkg/log/level.go Outdated Show resolved Hide resolved
internal/pkg/log/level.go Outdated Show resolved Hide resolved
internal/pkg/log/level.go Outdated Show resolved Hide resolved
@vitorhugoro1 vitorhugoro1 requested a review from MrAlias May 29, 2024 14:06
@MrAlias MrAlias modified the milestones: v0.12.0-alpha, v0.13.0-alpha Jun 4, 2024
@RonFed
Copy link
Contributor

RonFed commented Jun 13, 2024

@vitorhugoro1 bump 😄

@vitorhugoro1
Copy link
Contributor Author

Sorry for the late guys, applied the changes @MrAlias suggested and rebase with the main branch

level.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
instrumentation.go Outdated Show resolved Hide resolved
instrumentation.go Outdated Show resolved Hide resolved
instrumentation.go Outdated Show resolved Hide resolved
level.go Outdated Show resolved Hide resolved
level_test.go Show resolved Hide resolved
level_test.go Outdated Show resolved Hide resolved
@RonFed
Copy link
Contributor

RonFed commented Jun 26, 2024

@MrAlias can you please have another look?

@vitorhugoro1
Copy link
Contributor Author

@MrAlias can you review again, please?

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@MrAlias MrAlias merged commit e2d08c2 into open-telemetry:main Jul 1, 2024
18 checks passed
@MrAlias MrAlias mentioned this pull request Jul 15, 2024
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.

Add log verbosity configuration and move the log in controller.go to debug level
6 participants