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.

Sign up for GitHub

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

Jump to bottom

Add log verbosity configuration and move the log in controller.go to debug level #691

Open
RonFed opened this issue Feb 25, 2024 · 7 comments · May be fixed by #842
Open

Add log verbosity configuration and move the log in controller.go to debug level #691

RonFed opened this issue Feb 25, 2024 · 7 comments · May be fixed by #842
Labels
enhancement New feature or request go Pull requests that update Go code help wanted Extra attention is needed

Comments

@RonFed
Copy link
Contributor

RonFed commented Feb 25, 2024

Currently, we print a log message for each span:

opentelemetry-go-instrumentation/internal/pkg/opentelemetry/controller.go

Line 54 in 61ff124

c.logger.Info("got event", "attrs", event.Attributes, "status", event.Status)

We should change that to have a debug level.
We don't have a configuration for verbosity. We probably should add support for configuration via:

  1. command line arguments
  2. InstrumentationOption
  3. Enviroment variable (?)

This will need to be modified:

opentelemetry-go-instrumentation/instrumentation.go

Lines 70 to 83 in 61ff124

func newLogger() logr.Logger {
zapLog, err := zap.NewProduction()
var logger logr.Logger
if err != nil {
// Fallback to stdr logger.
logger = stdr.New(log.New(os.Stderr, "", log.LstdFlags))
} else {
logger = zapr.NewLogger(zapLog)
}
return logger
}

@RonFed RonFed added enhancement New feature or request help wanted Extra attention is needed go Pull requests that update Go code labels Feb 25, 2024
@RonFed
Copy link
Contributor Author

RonFed commented Mar 13, 2024

@MrAlias @pellared I'd be happy to hear your thoughts on this.

  • Can we add the logger as an Instrumentation option? I saw this comment about it:

    opentelemetry-go-instrumentation/instrumentation.go

    Lines 90 to 94 in 02686f0

    // TODO: pass this in as an option.
    //
    // We likely want to use slog instead of logr in the longterm. Wait until
    // that package has enough Go version support and then switch to that so we
    // can expose it in an option.
  • The log line mentioned in my first comment is the one I worried about since we currently have huge amounts of logs, so adding a verbosity option seems helpful (using the debug level for tests and development).

@blumamir blumamir mentioned this issue Mar 14, 2024
Open
@RonFed RonFed mentioned this issue Apr 21, 2024
Open
5 tasks
@khushijain21
Copy link

@RonFed is this open for contribution?

@RonFed
Copy link
Contributor Author

RonFed commented May 2, 2024

@khushijain21 Sure

@vitorhugoro1
Copy link

@khushijain21 are you going to work on this issue? If not I will work on this.

@RonFed from the three approches you provide, which one do you think is optimal?

By my experience when trying to use with k8s, if is with command line argument seems more clear which option of log level is set.

@khushijain21
Copy link

@vitorhugoro1 yes, you can go ahead

@RonFed
Copy link
Contributor Author

RonFed commented May 9, 2024

@vitorhugoro1 I think we need to support the first 2, and not sure about env var.
@MrAlias What do you think?

@vitorhugoro1 vitorhugoro1 linked a pull request May 13, 2024 that will close this issue
Open
@pellared
Copy link
Member

Regarding env var I think we should support OTEL_LOG_LEVEL per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request go Pull requests that update Go code help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
4 participants
@pellared @vitorhugoro1 @khushijain21 @RonFed