✨ Optimize EnvVar Middleware (Performance & Memory) #3218
Conversation
hexakleo
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes in the pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
middleware/envvar/envvar.go (2)
38-47: Consider adding request logging for security monitoring.Since this endpoint exposes environment variables, it would be beneficial to log access attempts for security monitoring purposes.
// Construct EnvVar and encode as JSON +if c.App().Config().EnableLogger { + c.Logger().Info("Environment variables accessed from IP: ", c.IP()) +} envVar := newEnvVar(cfg)
53-53: Consider pre-allocating map capacity for better performance.When the ExportVars is provided, we know the exact size needed for the map. Consider pre-allocating the capacity to avoid map growth and rehashing.
-envVar := &EnvVar{Vars: make(map[string]string)} +capacity := len(cfg.ExportVars) +if capacity == 0 { + capacity = len(os.Environ()) - len(cfg.ExcludeVars) +} +envVar := &EnvVar{Vars: make(map[string]string, capacity)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
middleware/envvar/envvar.go(1 hunks)
🔇 Additional comments (3)
middleware/envvar/envvar.go (3)
33-36: LGTM! Good security practice.
Restricting environment variable access to GET requests only is a good security practice, with appropriate status code handling.
69-71: LGTM! Excellent performance optimization.
Using strings.IndexByte instead of strings.SplitN is a great optimization for parsing environment variables, as it:
- Reduces allocations
- Performs faster for single character search
- Allows efficient string slicing
68-73: Verify handling of edge cases in environment variable parsing.
While using strings.IndexByte is a good performance optimization, ensure proper handling of edge cases:
- Empty keys (when idx = 0)
- Environment variables without '=' character
✅ Verification successful
Edge case handling for environment variables is properly implemented
The code's check if idx > 0 explicitly handles both edge cases mentioned in the review:
- Empty keys (when idx = 0) are properly skipped
- Variables without '=' are implicitly skipped as IndexByte returns -1
The test cases in middleware/envvar/envvar_test.go show proper testing of environment variables, and no test cases attempt to set empty keys or malformed environment variables, which aligns with the implementation's defensive approach of ignoring such cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any environment variables with empty keys or missing '=' in the codebase tests
rg -l "os.Setenv\([\"']" | xargs rg "os.Setenv\([\"']\s*[\"']"
Length of output: 70
Script:
#!/bin/bash
# Let's check the test files for environment variable test cases
rg -A 5 "os\.Setenv|t\.Setenv" --type go
# Also check if there are any test cases specifically for empty or invalid env vars
rg -A 5 "TestEnv|test.*Environment" --type go
# Look for actual implementation handling of edge cases
ast-grep --pattern 'if idx := strings.IndexByte($_, $_); $_ {
$$$
}'
Length of output: 2558
|
Can you create and share benchmark results to compare? |
Agree, we need an isolated benchmark to see the diff |
|
I don't think this PR improves the performance of the middleware. Here are the results: Old: Benchmark_newEnvVar-16 4691676 255.5 ns/op 344 B/op 3 allocs/op
New: Benchmark_newEnvVar-16 4552452 261.4 ns/op 344 B/op 3 allocs/opBenchmark Code: func Benchmark_newEnvVar(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for n := 0; n < b.N; n++ {
newEnvVar(Config{
ExportVars: map[string]string{"testKey": ""},
ExcludeVars: map[string]string{"excludeKey": ""},
})
}
} |
|
we will close it until a possible speed or memory improvement is confirmed thanks anyway for the idea to improve something |
Description
Note
The middleware was updated to only allow GET requests because its job is to return environment variables in JSON format, which is typically done with a GET request. Allowing methods like POST, PUT, or DELETE wasn't necessary and could cause unexpected behavior.
Now, the code explicitly checks for GET requests, and if the request is something else, it returns a 405 Method Not Allowed error:
Fixes # (issue)
N/A
Changes Introduced
strings.IndexByteinstead ofstrings.SplitN).Benchmarks:
Documentation Update:
Changelog/What's New:
Varsmap.Migration Guide:
Type of Change
Checklist
/docs/directory.Code Comparison
Old Code:
New Code: