Conversation
Adiciona o logger v2 para criar arquivos de log de nome único e que não rotacionam internamente
Summary of ChangesHello @joaoarthurlm, 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 significantly refactors the logging toolkit by introducing a modular, versioned approach. It separates the existing logging functionality into a dedicated V1 package and provides a new V2 implementation with a simplified file naming convention. A common utility for processing log content into JSON events has also been extracted, enhancing code reusability and maintainability. This change provides greater flexibility in choosing logging behaviors and streamlines the core logging interface. 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. Changelog
Activity
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 refactors the logging toolkit into V1 and V2 implementations, moving towards a more modular design with a new mchlogcommon package. However, it introduces several critical security vulnerabilities: both versions are susceptible to Path Traversal via the subject parameter, V1 uses insecure world-writable file and directory permissions, and V2 has a race condition leading to file descriptor leaks during concurrent logger initialization. Additionally, there are general issues concerning error handling, type conversion logic, and overall code clarity that need attention to ensure robustness and maintainability.
| var arr []any | ||
| if arr, ok = content.([]any); !ok { | ||
| tam := v.Len() | ||
| for i := 0; i < tam; i++ { | ||
| va := v.Index(i) | ||
| switch va.Kind() { | ||
| case reflect.String: | ||
| arr = append(arr, va.String()) | ||
| case reflect.Int, reflect.Int64, reflect.Int32: | ||
| arr = append(arr, va.Int()) | ||
| case reflect.Float64, reflect.Float32: | ||
| arr = append(arr, va.Float()) | ||
| } | ||
| } | ||
| } | ||
| event = logger.Log().Fields(arr) |
There was a problem hiding this comment.
The current logic for handling reflect.Slice or reflect.Array types (when not []byte or []any interpreted as key-value pairs) can lead to incorrect JSON output. If content is a strongly typed slice (e.g., []int{1, 2, 3}), it is converted to []any{1, 2, 3}. Passing this to logger.Log().Fields(arr) will cause zerolog to misinterpret the array elements as alternating keys and values, which is likely not the intended behavior and can result in malformed log entries.
To ensure correct JSON formatting, consider marshaling such slices/arrays to JSON and then unmarshaling them into a map[string]any if they represent a JSON object, or logging them as a single array field if they represent a JSON array of primitives. If the content cannot be converted to a valid JSON object, it should be explicitly treated as an invalid type.
| var arr []any | |
| if arr, ok = content.([]any); !ok { | |
| tam := v.Len() | |
| for i := 0; i < tam; i++ { | |
| va := v.Index(i) | |
| switch va.Kind() { | |
| case reflect.String: | |
| arr = append(arr, va.String()) | |
| case reflect.Int, reflect.Int64, reflect.Int32: | |
| arr = append(arr, va.Int()) | |
| case reflect.Float64, reflect.Float32: | |
| arr = append(arr, va.Float()) | |
| } | |
| } | |
| } | |
| event = logger.Log().Fields(arr) | |
| var arr []any | |
| if arr, ok = content.([]any); ok { // If content is already []any, assume it's key-value pairs | |
| event = logger.Log().Fields(arr) | |
| } else { | |
| // For other slice/array types (e.g., []int, []string), try to marshal to JSON | |
| // and then unmarshal to a map. If it's a simple array of primitives, | |
| // unmarshaling to a map will fail, correctly triggering the error. | |
| var tempBytes []byte | |
| tempBytes, err = json.Marshal(content) | |
| if err == nil { | |
| var m map[string]any | |
| if err = json.Unmarshal(tempBytes, &m); err == nil { | |
| event = logger.Log().Fields(m) | |
| } | |
| } | |
| } |
| if err != nil { | ||
| log.Error().Err(err).Interface("conteudo", subject).Msg("mchlogcorev2: falha ao processar conteúdo do log") | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to the file opening error, if mchlogcommon.GetJSONLogger returns an error, it is logged internally, but the LogSubject method returns silently. This means that if the content cannot be properly formatted into a JSON log event, the log entry is effectively dropped without the caller being informed. This can lead to critical information loss.
Evaluate whether silent failure is acceptable for this logging toolkit, or if a more explicit error handling strategy is needed to ensure log integrity.
| if obj, ok := l.mapLogger.Load(subject); ok { | ||
| logger = obj.(*zerolog.Logger) | ||
| } else { | ||
| // Se não estiver no cache, abre o arquivo e inicializa o logger | ||
| filename := filepath.Join(l.path, subject, subject+ccLogFileSuffix) | ||
|
|
||
| // Garante que a árvore de diretórios existe | ||
| _ = os.MkdirAll(filepath.Dir(filename), 0755) | ||
|
|
||
| // Abre o arquivo para escrita ao final (append). Cria se não existir. | ||
| f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0644) | ||
| if err != nil { | ||
| log.Error().Err(err).Str("filename", filename).Interface("conteudo", subject).Msg("mchlogcorev2: falha ao criar e abrir arquivo de log") | ||
| return | ||
| } | ||
|
|
||
| // Importante: Não fechamos o arquivo (f.Close) aqui pois ele será | ||
| // gerenciado pelo objeto logger que ficará no cache durante a vida do processo. | ||
| newLogger := zerolog.New(f).With().Timestamp().Logger() | ||
| logger = &newLogger | ||
| l.mapLogger.Store(subject, logger) |
There was a problem hiding this comment.
This block contains a race condition where multiple goroutines logging to a new subject simultaneously can lead to file descriptor leaks. Only the last one to call Store will have its logger cached, while others will leak their file descriptors. Additionally, the LogSubject method silently handles os.OpenFile errors, which can result in lost log data without the application being aware of the failure. To address these, use a sync.Mutex or sync.Map.LoadOrStore with a proper initialization pattern to ensure thread-safe logger creation, and consider a mechanism to notify callers of critical logging failures.
| ip = "-" + ip | ||
| } | ||
|
|
||
| logFile := filepath.Join(l.path, subject, subject+ip+"-"+dataHora+ccLogFileSuffix) |
There was a problem hiding this comment.
The subject parameter is used to construct the log file path without sanitization. An attacker who can control the subject string can use path traversal sequences (e.g., ..) to create directories and log files outside the intended base directory. It is recommended to sanitize the subject by using filepath.Base(subject) or by validating it against an allow-list.
| logger = obj.(*zerolog.Logger) | ||
| } else { | ||
| // Se não estiver no cache, abre o arquivo e inicializa o logger | ||
| filename := filepath.Join(l.path, subject, subject+ccLogFileSuffix) |
There was a problem hiding this comment.
The subject parameter is used to construct the log file path without sanitization, similar to the V1 implementation. This allows for path traversal attacks if the subject is derived from untrusted input. Consider sanitizing the input to ensure it does not contain path separators or traversal sequences.
| var err error | ||
| dirName := filepath.Dir(flt.filename) | ||
| if _, err = os.Stat(dirName); os.IsNotExist(err) { | ||
| err = os.MkdirAll(dirName, os.ModePerm) |
There was a problem hiding this comment.
Directories are created with os.ModePerm (0777), which is insecure as it allows any user on the system to create, delete, or modify files within the log directory. Use more restrictive permissions like 0750.
| default: | ||
| err = errors.New("") |
There was a problem hiding this comment.
The default error message errors.New("") is not descriptive and makes debugging difficult. It's important to provide clear and informative error messages to help identify the root cause of issues.
Please provide a more specific error message that indicates the type of content that was not handled.
| default: | |
| err = errors.New("") | |
| default: | |
| err = fmt.Errorf("unsupported reflect kind: %s", v.Kind().String()) |
| /* Exemplo de uso do mchlogcore. | ||
| // Primeiro o pacote precisa ser inicializado com o | ||
| // caminho a gravar os arquivos de log: | ||
| mchlogcore.InitializeMchLog("./etc/log") | ||
| // O pacote mchlogcore contém o objeto público MchLog e | ||
| // o único método acessível: LogSubject() | ||
| // Origem do log é uma string ou []byte em formato de json (tem que ser um json válido) | ||
| m := fmt.Sprintf("{\"somekey\":{\"id\":\"123\"},\"tick\":%d,\"time\":\"2020-11-09T12:05:19+01:00\",\"testmsg\":\"some msg\"}", 10) | ||
| mchlogcore.MchLog.LogSubject("teste", m, nil) | ||
| ou | ||
| mchlogcore.MchLog.LogSubject("teste", []byte(m), nil) | ||
| // output: {"somekey":{"id":"123"},"testmsg":"some msg","tick":10,"time":"2020-11-09T12:05:19+01:00","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste-169.254.215.202-2022101222.log | ||
| // Origem do log é um mapa onde a chave é uma string e o tipo do valor "dinâmico" (interface{}). | ||
| // A chave TEM que ser string, o valor pode ser string, int ou float. | ||
| // O tipo do valor pode ser estático também (string, por exemplo). | ||
| h := make(map[string]any) | ||
| h["mapa1"] = "aaa" | ||
| h["mapa2"] = 22 | ||
| mchlogcore.MchLog.LogSubject("teste", h, nil) | ||
| // output: {"mapa1":"aaa","mapa2":22,"data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste-169.254.215.202-2022101222.log | ||
| // Origem do log é um array/slice de tipo dinâmico. | ||
| // O tipo do array/slice pode ser estático, nesse caso só pode ser string. | ||
| // Começando do indice zero, os elementos pares são as "chaves" e | ||
| // os ímpares os valores, que podem ser string, int ou float. | ||
| var a []any | ||
| a = append(a, "key_array1", 22, "key_array2") | ||
| a = append(a, "55") | ||
| mchlogcore.MchLog.LogSubject("teste", a, nil) | ||
| // output: {"key_array1":22,"key_array2":"55","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste-169.254.215.202-2022101222.log | ||
| // Usando o log do exemplo anterior, desta vez enviando um erro. | ||
| mchlogcore.MchLog.LogSubject("teste", a, errors.New("isto é um erro forçado")) | ||
| // output: {"key_array1":22,"key_array2":"55","error":"isto é um erro forçado","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/err_teste/err_teste-169.254.215.202-2022101222.log |
There was a problem hiding this comment.
The extensive commented-out example code, while informative, adds significant verbosity to the file. This can make the actual code harder to read and maintain. Consider moving such detailed examples to a separate documentation file (e.g., a README.md in the package directory) or a dedicated _example directory, keeping the code files cleaner and focused on implementation.
| // Primeiro o pacote precisa ser inicializado com o | ||
| // caminho a gravar os arquivos de log: | ||
| mchlogcore.InitializeMchLog("./etc/log") | ||
| // O pacote mchlogcore contém o objeto público MchLog e | ||
| // o único método acessível: LogSubject() | ||
| // Origem do log é uma string ou []byte em formato de json (tem que ser um json válido) | ||
| m := fmt.Sprintf("{\"somekey\":{\"id\":\"123\"},\"tick\":%d,\"time\":\"2020-11-09T12:05:19+01:00\",\"testmsg\":\"some msg\"}", 10) | ||
| mchlogcore.MchLog.LogSubject("teste", m, nil) | ||
| ou | ||
| mchlogcore.MchLog.LogSubject("teste", []byte(m), nil) | ||
| // output: {"somekey":{"id":"123"},"testmsg":"some msg","tick":10,"time":"2020-11-09T12:05:19+01:00","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste.log | ||
| // Origem do log é um mapa onde a chave é uma string e o tipo do valor "dinâmico" (interface{}). | ||
| // A chave TEM que ser string, o valor pode ser string, int ou float. | ||
| // O tipo do valor pode ser estático também (string, por exemplo). | ||
| h := make(map[string]any) | ||
| h["mapa1"] = "aaa" | ||
| h["mapa2"] = 22 | ||
| mchlogcore.MchLog.LogSubject("teste", h, nil) | ||
| // output: {"mapa1":"aaa","mapa2":22,"data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste.log | ||
| // Origem do log é um array/slice de tipo dinâmico. | ||
| // O tipo do array/slice pode ser estático, nesse caso só pode ser string. | ||
| // Começando do indice zero, os elementos pares são as "chaves" e | ||
| // os ímpares os valores, que podem ser string, int ou float. | ||
| var a []any | ||
| a = append(a, "key_array1", 22, "key_array2") | ||
| a = append(a, "55") | ||
| mchlogcore.MchLog.LogSubject("teste", a, nil) | ||
| // output: {"key_array1":22,"key_array2":"55","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/teste/teste.log | ||
| // Usando o log do exemplo anterior, desta vez enviando um erro. | ||
| mchlogcore.MchLog.LogSubject("teste", a, errors.New("isto é um erro forçado")) | ||
| // output: {"key_array1":22,"key_array2":"55","error":"isto é um erro forçado","data_hora":"2022-10-12 19:11:33 UTC"} | ||
| // no arquivo ./etc/log/err_teste/err_teste.log |
There was a problem hiding this comment.
Similar to mchlogv1.go, the extensive commented-out example code here adds significant verbosity. Consider moving these detailed examples to external documentation or an _example directory to improve code readability and maintainability.
| // Importante: Não fechamos o arquivo (f.Close) aqui pois ele será | ||
| // gerenciado pelo objeto logger que ficará no cache durante a vida do processo. | ||
| newLogger := zerolog.New(f).With().Timestamp().Logger() | ||
| logger = &newLogger | ||
| l.mapLogger.Store(subject, logger) |
There was a problem hiding this comment.
The comment states that files are not closed because they are managed by the logger object in the cache for the lifetime of the process. While this can be efficient for a fixed and small number of log subjects, it can lead to an excessive number of open file descriptors if the subject parameter is dynamic and many unique subjects are logged over time. This could potentially exhaust system resources.
Consider adding a mechanism to explicitly close log files (e.g., a Close() method for the LogType or a cleanup routine) or implementing a strategy to periodically close and reopen less frequently used log files, especially if the number of unique subjects is not bounded.
2 participants
PR