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

[network path] Update dynamic path limits #33757

Merged
merged 7 commits into from
Feb 6, 2025
Merged

[network path] Update dynamic path limits #33757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7b03d36
[network path] Improve telemetry
AlexandreYang Feb 5, 2025
4138d52
monitor pathtest_processing_chan_size
AlexandreYang Feb 5, 2025
7404893
empty
AlexandreYang Feb 5, 2025
a914d98
add test
AlexandreYang Feb 5, 2025
77e7a3a
[network path] Update dynamic path limits
AlexandreYang Feb 5, 2025
053cafa
fix defaults
AlexandreYang Feb 5, 2025
cb99faf
Merge branch 'main' into alex/dynamic_path_limits
AlexandreYang Feb 5, 2025
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
6 changes: 3 additions & 3 deletions comp/networkpath/npcollector/npcollectorimpl/npcollector_test.go
View file
Open in desktop
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,9 @@ func Test_newNpCollectorImpl_defaultConfigs(t *testing.T) {

assert.Equal(t, true, npCollector.collectorConfigs.networkPathCollectorEnabled())
assert.Equal(t, 4, npCollector.workers)
assert.Equal(t, 100000, cap(npCollector.pathtestInputChan))
assert.Equal(t, 100000, cap(npCollector.pathtestProcessingChan))
assert.Equal(t, 100000, npCollector.collectorConfigs.pathtestContextsLimit)
assert.Equal(t, 1000, cap(npCollector.pathtestInputChan))
assert.Equal(t, 1000, cap(npCollector.pathtestProcessingChan))
assert.Equal(t, 5000, npCollector.collectorConfigs.pathtestContextsLimit)
assert.Equal(t, "default", npCollector.networkDevicesNamespace)
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/config/setup/config.go
View file
Open in desktop
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,9 @@ func InitConfig(config pkgconfigmodel.Setup) {
config.BindEnvAndSetDefault("network_path.collector.workers", 4)
config.BindEnvAndSetDefault("network_path.collector.timeout", DefaultNetworkPathTimeout)
config.BindEnvAndSetDefault("network_path.collector.max_ttl", DefaultNetworkPathMaxTTL)
config.BindEnvAndSetDefault("network_path.collector.input_chan_size", 100000)
config.BindEnvAndSetDefault("network_path.collector.processing_chan_size", 100000)
config.BindEnvAndSetDefault("network_path.collector.pathtest_contexts_limit", 100000)
config.BindEnvAndSetDefault("network_path.collector.input_chan_size", 1000)
config.BindEnvAndSetDefault("network_path.collector.processing_chan_size", 1000)
config.BindEnvAndSetDefault("network_path.collector.pathtest_contexts_limit", 5000)
config.BindEnvAndSetDefault("network_path.collector.pathtest_ttl", "15m")
config.BindEnvAndSetDefault("network_path.collector.pathtest_interval", "5m")
config.BindEnvAndSetDefault("network_path.collector.flush_interval", "10s")
Expand Down
6 changes: 3 additions & 3 deletions pkg/config/setup/config_test.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

can we consts that declared on the package?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AmitaiBl ,
Those default values are currently only used in config.go and not needed elsewhere (besides tests), I think it's fine to just keep the literal here, similar to many other cases in this config.go file.

About using the literal values in tests, I would preferred to use literal to check value, if we use constants that will make IMHO the test less reliable (having to rewrite the literal default value is a kind of double writing in accounting, it helps you verify/sanity check)

Copy link
Member Author

Choose a reason for hiding this comment

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

@AmitaiBl It's likely a matter of preference here,
so let me know if that's blocker, if so, I try to adjust :)

Original file line number Diff line number Diff line change
Expand Up @@ -692,9 +692,9 @@ func TestNetworkPathDefaults(t *testing.T) {
assert.Equal(t, 4, config.GetInt("network_path.collector.workers"))
assert.Equal(t, 1000, config.GetInt("network_path.collector.timeout"))
assert.Equal(t, 30, config.GetInt("network_path.collector.max_ttl"))
assert.Equal(t, 100000, config.GetInt("network_path.collector.input_chan_size"))
assert.Equal(t, 100000, config.GetInt("network_path.collector.processing_chan_size"))
assert.Equal(t, 100000, config.GetInt("network_path.collector.pathtest_contexts_limit"))
assert.Equal(t, 1000, config.GetInt("network_path.collector.input_chan_size"))
assert.Equal(t, 1000, config.GetInt("network_path.collector.processing_chan_size"))
assert.Equal(t, 5000, config.GetInt("network_path.collector.pathtest_contexts_limit"))
assert.Equal(t, 15*time.Minute, config.GetDuration("network_path.collector.pathtest_ttl"))
assert.Equal(t, 5*time.Minute, config.GetDuration("network_path.collector.pathtest_interval"))
assert.Equal(t, 10*time.Second, config.GetDuration("network_path.collector.flush_interval"))
Expand Down
Loading