Updates for automated linting config and various code fixes to match #815
Conversation
Summary of ChangesHello @drawks, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on modernizing the project's linting configuration, addressing code quality issues identified by the new linters, and refactoring the receiver plugin registration for better maintainability and testability. It also includes a fix for a race condition, enhancing the overall stability and reliability of the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements across the codebase, primarily focusing on modernizing the linting configuration, refactoring plugin registration for better determinism and testability, and enhancing error handling practices. The adoption of explicit Register() calls for receivers and consistent use of errors.Is and errors.As for error type checking greatly improves maintainability and robustness. Additionally, several potential resource leaks have been addressed by ensuring resp.Body.Close() calls, and a critical race condition in kafka.go has been resolved. Various minor typographical errors have also been corrected, contributing to overall code quality.
I am having trouble creating individual review comments. Click here to see my feedback.
receiver/kafka/kafka.go (415)
Moving rcv.waitGroup.Add(1) outside the goroutine is a critical fix for a potential race condition. If the goroutine starts and finishes very quickly, Done() could be called before Add(1), leading to a panic or incorrect WaitGroup behavior.
rcv.waitGroup.Add(1)
receiver/kafka/kafka.go (313)
Moving rcv.waitGroup.Add(1) outside the goroutine is a critical fix for a potential race condition. If the goroutine starts and finishes very quickly, Done() could be called before Add(1), leading to a panic or incorrect WaitGroup behavior.
rcv.waitGroup.Add(1)
carbonserver/flc.go (115-117)
The io.EOF error should be checked first, as it indicates the end of the stream and is not necessarily an actual error that should prevent further processing. This reordering ensures correct handling of io.EOF without masking other potential errors.
if errors.Is(err, io.EOF) {
return nil
}
tool/persister_configs_differ/main.go (62-64)
The io.EOF error should be checked first, as it indicates the end of the stream and is not necessarily an actual error that should prevent further processing. This reordering ensures correct handling of io.EOF without masking other potential errors.
if errors.Is(err, io.EOF) {
break
}
tags/tags.go (61)
Adding defer resp.Body.Close() is crucial for proper resource management. It ensures that the HTTP response body is always closed, preventing potential resource leaks.
defer resp.Body.Close()
receiver/parse/plain.go (88)
Using !errors.Is(err, io.EOF) is the correct and idiomatic way to check if an error is not io.EOF in Go, especially when err might be wrapped. This ensures that io.EOF is handled specifically without being treated as a general error.
if err != nil && !errors.Is(err, io.EOF) {
carbonserver/carbonserver.go (967-976)
The io.EOF error should be checked first, as it indicates the end of the stream and is not necessarily an actual error that should prevent further processing or trigger a fallback. This reordering ensures correct handling of io.EOF without masking other potential errors.
if errors.Is(err, io.EOF) {
break
}
if err != nil {
infos = append(infos, zap.NamedError("file_list_cache_read_error", err))
readFromCache = false
trieIdx = newTrie(".wsp", listener.maxCreatesPerSecond, listener.estimateSize)
break
}
receiver/kafka/kafka.go (383-388)
The errors.Is check for sarama.ErrOffsetOutOfRange should be performed before the general if err != nil check. This ensures that the specific offset out of range error is handled correctly without being caught by a broader error condition.
if errors.Is(err, sarama.ErrOffsetOutOfRange) {
rcv.logger.Error(
"kafka state offset out of range, restart from the oldest offset",
zap.Int64("kafka_state_offset", rcv.kafkaState.Offset),
)
partitionConsumer, err = consumer.ConsumePartition(rcv.connectOptions.topic, rcv.connectOptions.partition, sarama.OffsetOldest)
}
receiver/http/http_test.go (89)
Adding defer resp.Body.Close() is crucial for proper resource management. It ensures that the HTTP response body is always closed, preventing potential resource leaks.
defer resp.Body.Close()
points/reader.go (64-67)
The io.EOF error should be checked first, as it indicates the end of the stream and is not necessarily an actual error that should prevent further processing. This reordering ensures correct handling of io.EOF without masking other potential errors.
if errors.Is(err, io.EOF) {
break
}points/reader.go (22)
Using !errors.Is(err, io.EOF) is the correct and idiomatic way to check if an error is not io.EOF in Go, especially when err might be wrapped. This ensures that io.EOF is handled specifically without being treated as a general error.
if err != nil && !errors.Is(err, io.EOF) {
carbonserver/find.go (333)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
errors = append(errors, fmt.Errorf("%s: %w", expandedResult.Name, err))
persister/whisper_schema.go (105-106)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
return nil, fmt.Errorf("[persister] Failed to parse retentions %q for [%s]: %w",
schema.RetentionStr, schema.Name, err)
carbonserver/list.go (127)
Corrected a typo from "occured" to "occurred".
http.Error(wr, fmt.Sprintf("An internal error has occurred: %s", err), http.StatusInternalServerError)
carbonserver/list.go (298)
Corrected a typo from "occured" to "occurred".
http.Error(wr, fmt.Sprintf("An internal error has occurred: %s", err), http.StatusInternalServerError)
carbonserver/render.go (241)
Corrected a typo from "occured" to "occurred".
http.Error(wr, "Panic occurred, see logs for more information", http.StatusInternalServerError)
carbonserver/render.go (726)
Corrected a typo from "occured" to "occurred".
rpcErr = status.New(codes.Internal, "Panic occurred, see logs for more information").Err()
carbonserver/trie.go (585)
Corrected a typo from "unknwon" to "unknown".
return nil, &trieInsertError{typ: "failed to index metric: unknown case of match == nlen", info: fmt.Sprintf("match == nlen == %d", nlen)}
carbonserver/trie.go (2170)
The ps parameter was removed from maxCreatesThrottle in the function definition, so it should also be removed from its call site.
if ti.maxCreatesThrottle() {
persister/whisper.go (314)
Using errors.Is is the idiomatic way to check if an error matches a specific sentinel error, even if it's wrapped. This makes the error handling more robust and future-proof.
if errors.Is(err, syscall.ENAMETOOLONG) {
persister/whisper.go (578-579)
Using errors.As is the idiomatic way to check for a specific error type in Go, especially when dealing with wrapped errors. This makes the error handling more robust and future-proof.
var perr *os.PathError
if !errors.As(err, &perr) {
persister/whisper.go (582)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
return fmt.Errorf("%s: %w", perr.Op, perr.Err)
persister/whisper_quota.go (46)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
err = fmt.Errorf("[persister] Failed to parse %s for [%s]: %w", name, section["name"], err)
persister/whisper_schema.go (98-99)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
return nil, fmt.Errorf("[persister] Failed to parse pattern %q for [%s]: %w",
section["pattern"], schema.Name, err)
carbonserver/flc.go (163)
When errors.Is returns true, it implies that err is not nil. Therefore, the err != nil check is redundant and can be removed for cleaner code.
if errors.Is(err, errFLCFallbackToV1) {
persister/whisper_schema.go (113)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
return nil, fmt.Errorf("[persister] Failed to parse priority %q for [%s]: %w", section["priority"], schema.Name, err)
carbonserver/find.go (497-498)
Using errors.As is the idiomatic way to check for a specific error type in Go, especially when dealing with wrapped errors. This makes the error handling more robust and future-proof.
var nf errorNotFound
if errors.As(err, &nf) {
carbon/app.go (470)
Using %w for error wrapping is a best practice in Go, as it preserves the original error context, allowing for more robust error inspection using errors.Is and errors.As.
return fmt.Errorf("failed to stat whisper data directory: %w", err)
carbonserver/find.go (153-154)
Using errors.As is the idiomatic way to check for a specific error type in Go, especially when dealing with wrapped errors. This makes the error handling more robust and future-proof.
var nf errorNotFound
if errors.Is(err, &nf) {
carbonserver/details.go (110)
Corrected a typo from "occured" to "occurred".
http.Error(wr, fmt.Sprintf("An internal error has occurred: %s", err), http.StatusInternalServerError)
carbonserver/carbonserver_test.go (493)
Using errors.Is is the preferred way to check if an error matches a specific sentinel error, even if it's wrapped. This improves the robustness of error comparisons.
if !errors.Is(err, errMetricsListEmpty) {
carbonserver/carbonserver.go (1262-1263)
Using errors.As is the idiomatic way to check for a specific error type in Go, especially when dealing with wrapped errors. This makes the error handling more robust and future-proof.
var ierr *trieInsertError
if errors.As(err, &ierr) {
carbonserver/cache_index_test.go (188)
Corrected a typo from "recived" to "received".
fmt.Printf("error - received wrong point - %v\n", p2)
receiver/tcp/tcp.go (277)
Using errors.Is is the idiomatic way to check if an error matches a specific sentinel error, even if it's wrapped. This makes the error handling more robust and future-proof.
case errors.Is(err, io.EOF):
receiver/tcp/tcp.go (279)
Using errors.Is is the idiomatic way to check if an error matches a specific sentinel error, even if it's wrapped. This makes the error handling more robust and future-proof.
case errors.Is(err, framing.ErrPrefixLength):
carbonserver/cache_index_test.go (165)
Corrected a typo from "recived" to "received".
fmt.Printf("error - received wrong point - %v\n", p1)
carbon/app.go (488)
Consistent error wrapping with %w is good for maintainability, enabling callers to inspect the error chain.
return fmt.Errorf("failed to init Carbonserver.HeavyGlobQueryRateLimiters %s: %w", rl.Pattern, err)
* updates github action to use most recent `golangci-lint`
* migrates `.golangci.yml` for version 2+ compat
* enables some new linters
- asciicheck - no fails
- misspell - fixed spelling errors that caused fails
- promlinter - no fails
- errorlint - updated error string formats to us `%w` and updated
tests to match
- moves `waitGroup.Add(1)` outside of go routine - enables `govet` linter in `.golangci.yml`
* enables `unparam` linter in `glangci.yaml` * updates `render.go` and `trie.go` - removes unused `ps` param from `maxCreatesThrottle` - adds nolint:unparam and comments for other params that are unused
* enables `bodyclose` linter in `.golangci.yaml` * fixes unclosed http response bodies
* enables `gochecknoinits` linter in `.golintci.yaml * refactors receiver plugins to require explicit `Register` call instead of implicit `init` * updates `app.go` to explicitly register all receiver plugins guarded by `sync.Once` * updates recevier tests to explicitly call `Register` during `TestMain` * adds a `nolint:gochecknoinits` in `trie_test` to allow developer debug logging
|
@deniszh what do you think of this? The DeepSource: Go fails actually seem unrelated to any of these changes. I think they're just being triggered on files which were touched in these commits. I'm not sure if it makes sense to burn those down in the same PR or if it'd make more sense to update the deepsource config to ignore these as part of the "baseline" |
|
@drawks : you can ignore it, no worries |
* respolves CRT-A0001 - renames several `max` params to `m` to avoid shadowing
I pushed one commit to fix a simple mechanical thing from that deepsource report, but I'll stop pushing commits here if you think it looks good for merge. |
|
Let's merge this for now |
Updates for automated linting config and various code fixes to match
2 participants
PR Summary
This pull request modernizes and aligns the project’s linting configuration with the original intent documented in the existing config comments, while ensuring all functional behavior remains correct and fully covered by tests and linters.
Linting & Tooling Updates
Upgraded
golangci-lintto the latest version and migrated configuration for v2+ compatibility.Enabled linters that were already documented or implied in config comments:
gochecknoinitsbodycloseunparamgovetasciicheckmisspellpromlintererrorlintFixed all violations introduced by these linters (e.g., unclosed HTTP bodies, unused parameters, error wrapping formats, spelling issues, race conditions).
Functional Changes
Receiver plugin registration refactor (largest functional change):
init()-based plugin registration with explicitRegister()calls.app.go, guarded bysync.Once.TestMain.Fixed a
WaitGrouprace condition inkafka.goby movingAdd(1)outside goroutines.Guarantees