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

[bugfix] temporary fix around grpclogger #5272

Merged
merged 2 commits into from
Apr 28, 2022
Merged

[bugfix] temporary fix around grpclogger #5272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8994c2a
[bugfix] temporary fix around grpclogger
Apr 27, 2022
1aece5f
add SkipSettingGRPCLogger flag
Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion service/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,14 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error {
if err != nil {
return fmt.Errorf("failed to get config: %w", err)
}

if col.logger, err = telemetrylogs.NewLogger(cfg.Service.Telemetry.Logs, col.set.LoggingOptions); err != nil {
return fmt.Errorf("failed to get logger: %w", err)
}

telemetrylogs.SetColGRPCLogger(col.logger, cfg.Service.Telemetry.Logs.Level)
if !col.set.SkipSettingGRPCLogger {
telemetrylogs.SetColGRPCLogger(col.logger, cfg.Service.Telemetry.Logs.Level)
}

col.service, err = newService(&svcSettings{
BuildInfo: col.set.BuildInfo,
Expand Down
3 changes: 3 additions & 0 deletions service/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ type CollectorSettings struct {
// LoggingOptions provides a way to change behavior of zap logging.
LoggingOptions []zap.Option

// SkipSettingGRPCLogger avoids setting the grpc logger
SkipSettingGRPCLogger bool
Copy link
Member

Choose a reason for hiding this comment

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

I might still be in need of a coffee, but how does this work? This is a new value, and I see this value only being read, never written, so, it will always be false. The conditional on service/collector.go would then always yield true, which will call the same code as before this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be set in correctness tests, which are in the contrib repo, not in this repo.

BTW, I am not sure why we moved the all of the testbed from core to contrib. I consider the testbed to be a core part of the project. We run the testbed for otlp receiver/exporter in this repo, now we don't anymore and any problem discovery is delayed until the contrib consumes the new core.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree that maybe the framework of testbed can live here, the reason we moved it was because we don't want to give API guarantees to the framework, and core is closer and closer to offer also API stability.

Copy link
Member

Choose a reason for hiding this comment

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

the reason we moved it was because we don't want to give API guarantees to the framework

That's a good point. Maybe we can keep testbed go module on 0.x forever to indicate it is unstable and also have explicit warning that no-one should use this API except contrib (yes, I know people don't pay attention to warnings :-) ).


// For testing purpose only.
telemetry collectorTelemetryExporter
}