INFOPLAT-2282: Add beholderNoopLogerProvider and LogStreamingEnabled flag to control emitting logs #1263
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a toggle to enable or disable log streaming via the OTel exporter and provides a no-op logger provider when streaming is off.
- Added
BeholderNoopLoggerProviderto drop all logs when streaming is disabled. - Introduced
LogStreamingEnabledflag inConfigand wired it through both HTTP and GRPC clients. - Updated default configuration and tests to account for the new flag.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/beholder/noop.go | Added beholderNoopLogExporter and BeholderNoopLoggerProvider. |
| pkg/beholder/httpclient.go | Conditional logger setup based on LogStreamingEnabled. |
| pkg/beholder/config_test.go | Updated ExampleConfig to include LogStreamingEnabled. |
| pkg/beholder/config.go | Added LogStreamingEnabled field and default in DefaultConfig. |
| pkg/beholder/client_test.go | Tests for enabling/disabling log streaming. |
| pkg/beholder/client.go | Conditional logger setup in GRPC client based on LogStreamingEnabled. |
Comments suppressed due to low confidence (2)
pkg/beholder/config.go:49
- [nitpick] The comment on LogStreamingEnabled could mention its default value (
false) as defined in DefaultConfig to clarify its default behavior.
LogStreamingEnabled bool // Enable streaming logs to the OTel log exporter
pkg/beholder/config_test.go:50
- [nitpick] Consider adding a unit test to verify that
DefaultConfig().LogStreamingEnabledisfalseto ensure default behavior remains consistent.
LogStreamingEnabled: false, // Disable streaming logs by default
pkcll
reviewed
Jun 13, 2025
pkg/beholder/client.go
Outdated
| } | ||
| if cfg.LogExportMaxBatchSize > 0 { | ||
| batchProcessorOpts = append(batchProcessorOpts, sdklog.WithExportMaxBatchSize(cfg.LogExportMaxBatchSize)) // Default is 512, must be <= maxQueueSize | ||
| var loggerProvider *sdklog.LoggerProvider |
There was a problem hiding this comment.
nit: get rid of else
Suggested change
| var loggerProvider *sdklog.LoggerProvider | |
| loggerProvider := BeholderNoopLoggerProvider() |
and remove
else {
loggerProvider = BeholderNoopLoggerProvider()
}
or to keep code more flat
just add
if !cfg.LogStreamingEnabled {
loggerProvider = BeholderNoopLoggerProvider()
}
Note: we expect it to be normally alway enabled in the future
pkcll
reviewed
Jun 13, 2025
pkg/beholder/config.go
Outdated
| // Retry config for shared log exporter, used by Emitter and Logger | ||
| LogRetryConfig *RetryConfig | ||
| LogRetryConfig *RetryConfig | ||
| LogStreamingEnabled bool // Enable streaming logs to the OTel log exporter |
There was a problem hiding this comment.
nit: // Enable logs streaming via OTel Logger
pkcll
reviewed
Jun 13, 2025
pkg/beholder/config.go
Outdated
| LogExportInterval: 1 * time.Second, | ||
| LogMaxQueueSize: 2048, | ||
| LogBatchProcessor: true, | ||
| LogStreamingEnabled: false, // Disable streaming logs by default |
There was a problem hiding this comment.
nit: streaming logs -> logs streaming since we use that terminology across the board
NOTE: for the config option singular form LogStreamingEnabled makes more sense for consistency with other log config options
pkcll
reviewed
Jun 13, 2025
pkg/beholder/httpclient.go
Outdated
| if cfg.LogExportMaxBatchSize > 0 { | ||
| batchProcessorOpts = append(batchProcessorOpts, sdklog.WithExportMaxBatchSize(cfg.LogExportMaxBatchSize)) // Default is 512, must be <= maxQueueSize | ||
| var loggerProvider *sdklog.LoggerProvider | ||
| if cfg.LogStreamingEnabled { |
There was a problem hiding this comment.
pkcll
previously approved these changes
Jun 13, 2025
pavel-raykov
approved these changes
Jun 16, 2025
fouadkada
approved these changes
Jun 16, 2025
jinhoonbang
pushed a commit
that referenced
this pull request
Jun 17, 2025
…flag to control emitting logs (#1263) * Add beholderNoopLogerProvider and LogStreamingEnabled flag to control emitting logs * Fix tests * Fix config test * Flatten if conditions * Fix tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds
beholderNoopLogerProviderandLogStreamingEnabledflag to control emitting logs throughsharedLogExporterThe ticket for the reference: https://smartcontract-it.atlassian.net/browse/INFOPLAT-2282