Add PostHog writer #1
Conversation
WalkthroughAdds a PostHog-backed writer for golog (module, README, constructors, pooled Writer, context-based disable helpers), environment and client configuration, example and Docker integration tests with compose and runner, plus logfile rotating-writer API and tests and multiple go.mod dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant golog as golog.Logger
participant WC as posthog.WriterConfig
participant W as posthog.Writer
participant PH as PostHog Client
App->>golog: log(ctx, level).With(...).Msg("text")
golog->>WC: WriterForNewMessage(ctx, level)
alt ContextWithoutLogging present
WC-->>golog: nil
golog-->>App: skip capture
else
WC->>W: obtain writer (pool/new)
golog->>W: BeginMessage(cfg, ts, level, prefix, text)
loop write values
golog->>W: WriteKey/WriteX(...)
end
golog->>W: CommitMessage()
W->>PH: Capture(event, distinctId, properties, timestamp)
W-->>WC: reset and return to pool
end
sequenceDiagram
autonumber
participant App
participant Config as posthog/config.go
participant golog as golog.Logger
participant WC as posthog.WriterConfig
App->>Config: ContextWithoutLogging(parent)
Config-->>App: ctx' (with marker)
App->>golog: log(ctx', ...)
golog->>WC: WriterForNewMessage(ctx', level)
WC-->>golog: nil (logging disabled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to pay attention to:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
posthog/config.go (1)
5-7: Use an unexported struct type for the context key.Using an
intas a context key can lead to collisions with other packages. Best practice is to use an unexported struct type to ensure uniqueness.Apply this diff:
-var ( - withoutLoggingCtxKey int -) +type contextKey struct{} + +var ( + withoutLoggingCtxKey = contextKey{} +)Then update the usage to use the value directly instead of its pointer:
- return context.WithValue(parent, &withoutLoggingCtxKey, struct{}{}) + return context.WithValue(parent, withoutLoggingCtxKey, struct{}{})- return ctx != nil && ctx.Value(&withoutLoggingCtxKey) != nil + return ctx != nil && ctx.Value(withoutLoggingCtxKey) != nilposthog/writer.go (1)
68-70: Consider renaming or documenting the behavior.The method name
FlushUnderlyingsuggests it only flushes pending events, butclient.Close()also terminates the client. This could be misleading to callers who might think they can continue using the writer after flushing.Consider either:
- Renaming to
CloseUnderlyingto match the actual behavior- Adding a comment explaining that this method closes the client and no further logging is possible
+// FlushUnderlying flushes pending events and closes the PostHog client. +// The writer cannot be used after calling this method. func (c *WriterConfig) FlushUnderlying() { c.client.Close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumposthog/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
posthog/README.md(1 hunks)posthog/config.go(1 hunks)posthog/example_test.go(1 hunks)posthog/go.mod(1 hunks)posthog/writer.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
posthog/example_test.go (6)
posthog/writer.go (1)
NewWriterConfigFromEnv(33-44)format.go (1)
NewDefaultFormat(11-19)levelfilter.go (1)
AllLevelsActive(9-9)logger.go (1)
NewLogger(21-29)config.go (1)
NewConfig(43-56)levels.go (1)
DefaultLevels(9-24)
posthog/writer.go (4)
levelfilter.go (1)
LevelFilter(17-17)level.go (1)
Level(9-9)posthog/config.go (1)
IsContextWithoutLogging(21-23)uuid.go (1)
FormatUUID(28-40)
🪛 LanguageTool
posthog/README.md
[grammar] ~7-~7: There might be a mistake here.
Context: ... - Sends log messages as PostHog events - Supports structured logging with key-val...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ... structured logging with key-value pairs - Configurable extra properties for all lo...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...le extra properties for all log messages - Proper level name handling - Writer pool...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...og messages - Proper level name handling - Writer pooling for performance - Context...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...andling - Writer pooling for performance - Context-based logging control ## Usage ...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...lude key-value pairs in the message text - extra: Map of extra properties added to every...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ... PostHog event with: - Event Name: "log_message" - Distinct ID: "system" (configurable)...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...Distinct ID**: "system" (configurable) - Properties: - message: The log mes...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...ystem"(configurable) - **Properties**: -message: The log message text - log_level`: ...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...s**: - message: The log message text - log_level: The log level name (e.g., "INFO", "ERR...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...e log level name (e.g., "INFO", "ERROR") - Additional properties from extra confi...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ...dditional properties from extra config - Key-value pairs from structured logging ...
(QB_NEW_EN)
🔇 Additional comments (4)
posthog/writer.go (3)
58-66: LGTM! Good use of early returns and pooling.The method correctly checks both the level filter and context-based logging control before allocating resources. The pool retrieval with type assertion is properly handled.
138-144: LGTM!The key handling is correct and properly supports both modes (values in properties only vs. values in message text).
200-238: LGTM! Efficient pooling and formatting.The value writing logic correctly handles both slice accumulation and final storage, with proper map pooling to reduce allocations. The comma logic for slice formatting is correct.
posthog/go.mod (1)
7-7: Confirm PostHog-go version publication and stability. go.mod pins v1.6.11 but GitHub’s latest release is v1.6.10 and no advisories exist. Ensure v1.6.11 is correctly tagged/published and stable; otherwise revert to v1.6.10.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
posthog/writer.go (1)
46-49: Trim whitespace from the API key.The validation checks for an empty string but doesn't trim leading/trailing whitespace. An API key consisting only of whitespace would pass validation but fail when used.
Apply this diff:
- apiKey := os.Getenv("POSTHOG_API_KEY") - if apiKey == "" { + apiKey := strings.TrimSpace(os.Getenv("POSTHOG_API_KEY")) + if apiKey == "" { return nil, errors.New("POSTHOG_API_KEY is not set")
🧹 Nitpick comments (4)
posthog/writer.go (4)
52-54: Consider making the PostHog endpoint configurable.The endpoint is hardcoded to the US region (
https://us.i.posthog.com). Users in other regions (e.g., EU athttps://eu.i.posthog.com) or those using self-hosted PostHog would need to modify the code.Consider reading the endpoint from an environment variable with a fallback:
+ endpoint := os.Getenv("POSTHOG_ENDPOINT") + if endpoint == "" { + endpoint = "https://us.i.posthog.com" // Default endpoint + } client, err := posthog.NewWithConfig( apiKey, posthog.Config{ - Endpoint: "https://us.i.posthog.com", // Default endpoint + Endpoint: endpoint, }, )
74-83: Consider validating the distinctID against PostHog restrictions.The documentation (lines 72-73) mentions that PostHog restricts certain values like "anonymous", "guest", "distinctid", "undefined", "null", "true", "false", etc., but the constructor doesn't validate the provided distinctID against these restrictions. Invalid values could cause issues with PostHog analytics.
Consider adding validation:
func NewWriterConfig(client posthog.Client, format *golog.Format, filter golog.LevelFilter, distinctID string, valsAsMsg bool, extra map[string]any) *WriterConfig { // Validate distinctID against PostHog restrictions restrictedIDs := map[string]bool{ "anonymous": true, "guest": true, "distinctid": true, "undefined": true, "null": true, "true": true, "false": true, } if restrictedIDs[strings.ToLower(distinctID)] { // Consider logging a warning or returning an error } return &WriterConfig{ format: format, filter: filter, client: client, distinctID: distinctID, valsAsMsg: valsAsMsg, extra: extra, } }
143-148: Consider error handling for PostHog event capture.The
Enqueuemethod is called without any error handling. While the posthog-go client uses asynchronous queueing, it can still fail silently if the queue is full or the client encounters issues. Consider whether you need to handle failures, perhaps by logging them.The posthog-go library doesn't return errors from
Enqueue(it's fire-and-forget), but you might want to verify this behavior and document it:// Enqueue is fire-and-forget; errors are handled internally by the PostHog client w.config.client.Enqueue(posthog.Capture{ DistinctId: w.config.distinctID, Event: "log_message", Timestamp: w.timestamp, Properties: properties.Set("message", w.message.String()), })Alternatively, check if the PostHog client provides any callback mechanism for capture failures.
145-145: Hardcoded event name limits flexibility.The event name is hardcoded as
"log_message". Different log levels or categories might benefit from distinct event names (e.g.,"log_error","log_info"), which would improve PostHog analytics and filtering.Consider making the event name configurable or deriving it from the log level:
eventName := "log_" + strings.ToLower(levelName) w.config.client.Enqueue(posthog.Capture{ DistinctId: w.config.distinctID, Event: eventName, Timestamp: w.timestamp, Properties: properties.Set("message", w.message.String()), })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumposthog/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
posthog/README.md(1 hunks)posthog/example_test.go(1 hunks)posthog/go.mod(1 hunks)posthog/writer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- posthog/example_test.go
- posthog/README.md
🧰 Additional context used
🧬 Code graph analysis (1)
posthog/writer.go (4)
levelfilter.go (1)
LevelFilter(17-17)level.go (1)
Level(9-9)posthog/config.go (1)
IsContextWithoutLogging(21-23)uuid.go (1)
FormatUUID(28-40)
🔇 Additional comments (1)
posthog/go.mod (1)
3-3: Review comment is incorrect and should be dismissed.Go 1.24 was released on February 11, 2025, and Go 1.24.4 is the latest stable version as of June 2025. The
go 1.24.0requirement in the file is valid and will not cause compatibility issues or build failures. No action is required.Likely an incorrect or invalid review comment.
commit fe2c1bb Author: Erik Unger <[email protected]> Date: Wed Oct 22 13:33:32 2025 +0200 Update PostHog integration: bump posthog-go to v1.6.12, add distinctID parameter for event tracking, and enhance README with distinctID usage examples and restrictions. commit 8bbe7c6 Author: Erik Unger <[email protected]> Date: Wed Oct 15 14:34:06 2025 +0200 generated posthog implementation commit 08c9683 Author: Erik Unger <[email protected]> Date: Wed Jun 11 16:02:25 2025 +0200 start implementation
…, logger, and callback options. Update NewWriterConfigFromEnv to utilize environment variables for endpoint and default properties, improving flexibility in client setup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
posthog/writer.go (3)
33-72: Add validation for the distinctID parameter.The function documents that PostHog restricts certain values (
"anonymous","guest","distinctid","undefined","null","true","false", etc.) and that the distinctID should be unique and meaningful. However, there's no validation to prevent empty or restricted values from being used.Consider adding validation before creating the client:
func NewWriterConfigFromEnv(format *golog.Format, filter golog.LevelFilter, distinctID string, valsAsMsg bool, defaultProps posthog.Properties) (*WriterConfig, error) { apiKey := strings.TrimSpace(os.Getenv("POSTHOG_API_KEY")) if apiKey == "" { return nil, errors.New("POSTHOG_API_KEY is not set") } + distinctID = strings.TrimSpace(distinctID) + if distinctID == "" { + return nil, errors.New("distinctID cannot be empty") + } + // Check for restricted values + restricted := []string{"anonymous", "guest", "distinctid", "undefined", "null", "true", "false"} + for _, r := range restricted { + if strings.EqualFold(distinctID, r) { + return nil, fmt.Errorf("distinctID %q is restricted by PostHog", distinctID) + } + } endpoint := cmp.Or( strings.TrimSpace(os.Getenv("POSTHOG_ENDPOINT")), DefaultEndpoint, )
74-94: Add validation for the distinctID parameter.Similar to
NewWriterConfigFromEnv, this constructor should validate thatdistinctIDis not empty and doesn't use restricted values.func NewWriterConfig(client posthog.Client, format *golog.Format, filter golog.LevelFilter, distinctID string, valsAsMsg bool) *WriterConfig { + distinctID = strings.TrimSpace(distinctID) + if distinctID == "" { + panic("distinctID cannot be empty") + } + // Optional: Check for restricted values as shown in previous comment return &WriterConfig{ format: format, filter: filter, client: client, distinctID: distinctID, valsAsMsg: valsAsMsg, } }Note: Since this doesn't return an error, you could either panic (as constructors sometimes do for invalid inputs) or document that the caller must ensure distinctID is valid.
133-165: LGTM with optional clarity improvement.The message commit logic correctly builds PostHog properties, enqueues the event, and cleans up resources. The pooling strategy is efficient.
Optional: For clarity, you could separate the message property assignment:
// Capture the log event + properties.Set("message", w.message.String()) w.config.client.Enqueue(posthog.Capture{ DistinctId: w.config.distinctID, Event: "log_message", Timestamp: w.timestamp, - Properties: properties.Set("message", w.message.String()), + Properties: properties, })This avoids chaining and makes the property assignment more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
posthog/config.go(1 hunks)posthog/writer.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T11:58:39.493Z
Learnt from: ungerik
PR: domonda/golog#1
File: posthog/writer.go:95-97
Timestamp: 2025-10-22T11:58:39.493Z
Learning: In posthog/writer.go, the FlushUnderlying() method on WriterConfig should not call c.client.Close(). Suggesting to call c.client.Close() in FlushUnderlying() for the PostHog writer is a false positive.
Applied to files:
posthog/writer.go
🧬 Code graph analysis (1)
posthog/writer.go (2)
posthog/config.go (9)
DefaultEndpoint(14-14)DefaultEventProperties(36-36)DefaultInterval(19-19)DefaultTransport(25-25)DefaultLogger(31-31)DefaultCallback(40-40)DefaultBatchSize(47-47)DefaultVerbose(51-51)IsContextWithoutLogging(66-68)uuid.go (1)
FormatUUID(28-40)
🔇 Additional comments (1)
posthog/config.go (1)
19-51: Minor grammar correction needed on line 28.The zero-valued defaults (e.g.,
DefaultInterval,DefaultBatchSize) are appropriate; posthog-go's built-in defaults are 5s for interval and 250 for batch size, which will be used when these are not explicitly configured.Update the comment on line 28 from "that are generated" to "that is generated" (or restructure: "when generated by background operations").
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
posthog/example_test.go (1)
44-47: Avoid logging PII in examples.The email address remains in the example code. Examples should demonstrate compliance best practices by avoiding PII logging.
Apply this diff as previously suggested:
logger.NewMessage(ctx, golog.DefaultLevels.Info, "User logged in"). Str("user_id", "12345"). - Str("email", "[email protected]"). + Str("action_type", "authentication"). Log()
🧹 Nitpick comments (1)
posthog/docker-compose.yml (1)
4-5: Consider pinning the PostHog image version.Using
latestmakes builds non-deterministic and can lead to unexpected behavior when the image is updated. For reproducible tests and environments, consider pinning to a specific version.Apply this diff to pin to a specific version:
posthog: - image: posthog/posthog:latest + image: posthog/posthog:1.50.0 # Or latest stable version container_name: posthog-test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
posthog/DOCKER_TEST_README.md(1 hunks)posthog/docker-compose.yml(1 hunks)posthog/docker_test.go(1 hunks)posthog/example_test.go(1 hunks)posthog/run-docker-test.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
posthog/example_test.go (7)
posthog/writer.go (1)
NewWriterConfigFromEnv(46-72)format.go (1)
NewDefaultFormat(11-19)levelfilter.go (1)
AllLevelsActive(9-9)logger.go (1)
NewLogger(21-29)config.go (1)
NewConfig(43-56)levels.go (1)
DefaultLevels(9-24)attrib.go (1)
Int(281-284)
posthog/run-docker-test.sh (1)
posthog/docker_test.go (2)
TestPostHogWithDocker(60-88)TestPostHogLogFile(388-467)
posthog/docker_test.go (5)
posthog/writer.go (1)
NewWriterConfigFromEnv(46-72)format.go (1)
NewDefaultFormat(11-19)levelfilter.go (1)
AllLevelsActive(9-9)logger.go (1)
NewLogger(21-29)config.go (1)
NewConfig(43-56)
🪛 Checkov (3.2.334)
posthog/docker-compose.yml
[medium] 11-12: Basic Auth Credentials
(CKV_SECRET_4)
🪛 LanguageTool
posthog/DOCKER_TEST_README.md
[style] ~22-~22: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
[style] ~29-~29: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
[style] ~36-~36: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
🪛 markdownlint-cli2 (0.18.1)
posthog/DOCKER_TEST_README.md
24-24: Multiple headings with the same content
(MD024, no-duplicate-heading)
31-31: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (16)
posthog/example_test.go (6)
1-10: LGTM!Package declaration and imports are appropriate for the example.
12-16: LGTM!Environment setup is appropriate for an example function. The comment correctly notes that actual tests should use
t.Setenv()for better isolation.
18-21: LGTM!Default properties demonstrate appropriate metadata fields without sensitive information.
23-33: LGTM!Writer configuration demonstrates proper usage of
NewWriterConfigFromEnvwith clear inline documentation for each parameter.
35-40: LGTM!Logger creation correctly demonstrates integration of the PostHog writer with golog using consistent configuration.
49-52: LGTM!Error log message demonstrates appropriate operational logging with relevant technical context and no PII.
posthog/run-docker-test.sh (3)
1-19: LGTM!The script header and Docker availability checks are well-structured with clear error messages and appropriate error handling via
set -e.
21-39: Test orchestration looks good.The script properly manages the Docker environment lifecycle. The hardcoded 30-second sleep (line 32) provides an initial wait, while the Go tests in
docker_test.gohave proper health checks viawaitForPostHogto ensure readiness before proceeding.
41-56: LGTM!The log file handling gracefully manages missing files and provides a useful preview before cleanup. The approach of showing the first 50 lines strikes a good balance between visibility and brevity.
posthog/docker-compose.yml (1)
37-63: LGTM!The PostgreSQL and Redis service configurations are standard and well-configured with appropriate health checks and persistent volumes.
posthog/docker_test.go (6)
18-57: LGTM!The log file helper functions are well-implemented with proper error handling and automatic cleanup via
t.Cleanup(). The polling interval inwaitForLogContent(100ms) provides a good balance between responsiveness and resource usage.
59-129: LGTM!The Docker test orchestration is robust with proper:
- Skip conditions for CI and missing Docker
- Cleanup via defer and t.Cleanup
- Health check polling with appropriate retries
- Response body cleanup in both success and error paths
131-230: LGTM!The basic logging test comprehensively covers different log levels, structured logging, and context-based control. The 2-second sleep (line 205) provides time for event processing, which is appropriate for integration tests. The log analysis is appropriately lenient for test reliability.
232-288: LGTM!The configuration validation tests thoroughly cover edge cases (missing, empty, valid API keys) with proper assertions on error messages and config state.
290-328: LGTM!The table-driven DistinctId validation test provides good coverage of different ID patterns. The comments clarify that empty, anonymous, and guest IDs are intentionally allowed in the implementation.
330-467: LGTM!The test suite effectively separates unit tests of helper functions from integration tests with Docker. The lenient assertions in
TestPostHogLogFile(logging warnings rather than failing on missing patterns) make the tests resilient to PostHog startup variations while still validating core functionality.
| ## Test Features | ||
|
|
||
| ### Docker Integration | ||
| - **Automatic container management**: Starts and stops PostHog services automatically | ||
| - **Health checks**: Waits for PostHog to be ready before running tests | ||
| - **Cleanup**: Properly cleans up containers and volumes after tests | ||
|
|
||
| ## Test Features | ||
|
|
||
| ### Docker Integration | ||
| - **Automatic container management**: Starts and stops PostHog services automatically | ||
| - **Health checks**: Waits for PostHog to be ready before running tests | ||
| - **Cleanup**: Properly cleans up containers and volumes after tests | ||
|
|
||
| ## Test Features | ||
|
|
||
| ### Docker Integration | ||
| - **Automatic container management**: Starts and stops PostHog services automatically | ||
| - **Health checks**: Waits for PostHog to be ready before running tests | ||
| - **Cleanup**: Properly cleans up containers and volumes after tests |
There was a problem hiding this comment.
Remove duplicate sections.
The document contains three identical "## Test Features" headings (lines 24, 31) with duplicate "### Docker Integration" content. This appears to be a copy-paste error.
Apply this diff to remove the duplicate sections:
- Updated `example_test.go` - Updated example to match new API
## Test Features
### Docker Integration
- **Automatic container management**: Starts and stops PostHog services automatically
- **Health checks**: Waits for PostHog to be ready before running tests
- **Cleanup**: Properly cleans up containers and volumes after tests
-## Test Features
-
-### Docker Integration
-- **Automatic container management**: Starts and stops PostHog services automatically
-- **Health checks**: Waits for PostHog to be ready before running tests
-- **Cleanup**: Properly cleans up containers and volumes after tests
-
-## Test Features
-
-### Docker Integration
-- **Automatic container management**: Starts and stops PostHog services automatically
-- **Health checks**: Waits for PostHog to be ready before running tests
-- **Cleanup**: Properly cleans up containers and volumes after tests
-
### Log File Management📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Test Features | |
| ### Docker Integration | |
| - **Automatic container management**: Starts and stops PostHog services automatically | |
| - **Health checks**: Waits for PostHog to be ready before running tests | |
| - **Cleanup**: Properly cleans up containers and volumes after tests | |
| ## Test Features | |
| ### Docker Integration | |
| - **Automatic container management**: Starts and stops PostHog services automatically | |
| - **Health checks**: Waits for PostHog to be ready before running tests | |
| - **Cleanup**: Properly cleans up containers and volumes after tests | |
| ## Test Features | |
| ### Docker Integration | |
| - **Automatic container management**: Starts and stops PostHog services automatically | |
| - **Health checks**: Waits for PostHog to be ready before running tests | |
| - **Cleanup**: Properly cleans up containers and volumes after tests | |
| ## Test Features | |
| ### Docker Integration | |
| - **Automatic container management**: Starts and stops PostHog services automatically | |
| - **Health checks**: Waits for PostHog to be ready before running tests | |
| - **Cleanup**: Properly cleans up containers and volumes after tests | |
| ### Log File Management |
🧰 Tools
🪛 LanguageTool
[style] ~22-~22: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
[style] ~29-~29: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
[style] ~36-~36: Consider an alternative adverb to strengthen your wording.
Context: ...ady before running tests - Cleanup: Properly cleans up containers and volumes after ...
(PROPERLY_THOROUGHLY)
🪛 markdownlint-cli2 (0.18.1)
24-24: Multiple headings with the same content
(MD024, no-duplicate-heading)
31-31: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
posthog/DOCKER_TEST_README.md around lines 17 to 36: the file contains three
identical "## Test Features" sections with duplicated "### Docker Integration"
bullet lists (copy-paste error); remove the two extra duplicate sections so only
one "## Test Features" with a single "### Docker Integration" block and its
three bullets remain, ensuring spacing/newlines are adjusted to keep the
document formatting consistent.
| 3. **De | ||
| bug Logging**: Debug messages with detailed information |
There was a problem hiding this comment.
Fix line break in "Debug Logging".
The text "De\nbug Logging" is split across two lines, which appears to be a formatting error.
Apply this diff:
1. **Info Logging**: Basic informational messages with structured data
2. **Error Logging**: Error messages with error codes and retry counts
-3. **De
-bug Logging**: Debug messages with detailed information
+3. **Debug Logging**: Debug messages with detailed information
4. **Structured Logging**: Complex user action logging with multiple fields📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. **De | |
| bug Logging**: Debug messages with detailed information | |
| 1. **Info Logging**: Basic informational messages with structured data | |
| 2. **Error Logging**: Error messages with error codes and retry counts | |
| 3. **Debug Logging**: Debug messages with detailed information | |
| 4. **Structured Logging**: Complex user action logging with multiple fields |
🤖 Prompt for AI Agents
In posthog/DOCKER_TEST_README.md around lines 61 to 62, the phrase "De\nbug
Logging" is broken across two lines; fix it by removing the newline so the
heading reads "Debug Logging" on a single line, preserving surrounding
formatting and spacing.
…tation. Added examples for usage and configuration, including thread safety and file management. Updated tests to cover various rotation scenarios and improved error handling in the RotatingWriter implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logfile/rotatingwriter.go (1)
145-159: Fix rotation condition and size accounting in Write.
- Don’t mutate
rw.sizebefore writing.- Rotate when current size ≥ threshold (matches tests).
- Increment size by actual
nwritten.Apply:
-func (rw *RotatingWriter) Write(msg []byte) (n int, err error) { - rw.mtx.Lock() - defer rw.mtx.Unlock() - - rw.size += int64(len(msg)) - if rw.rotateSize > 0 && rw.size >= rw.rotateSize { - err := rw.rotate() - if err != nil { - return 0, err - } - rw.size += int64(len(msg)) - } - - return rw.file.Write(msg) -} +func (rw *RotatingWriter) Write(p []byte) (int, error) { + rw.mtx.Lock() + defer rw.mtx.Unlock() + + // Rotate if the current file has reached/exceeded threshold. + if rw.rotateSize > 0 && rw.size >= rw.rotateSize { + if err := rw.rotate(); err != nil { + return 0, err + } + } + n, err := rw.file.Write(p) + rw.size += int64(n) + return n, err +}
🧹 Nitpick comments (5)
README.md (1)
186-191: Snippet fix:log.Fatalwithout import.Either import the stdlib
logor switch topanic(err)for brevity in README snippets.Example change:
-if err != nil { - log.Fatal(err) -} +if err != nil { + panic(err) +}logfile/rotatingwriter_test.go (2)
357-369: Fragile expectation: exact file-count based on assumed line sizes.Hard-coding counts via “2 messages per file” can flake if formatting changes. Prefer asserting “>= 1 rotation” and verifying total message count across files, or compute expected by file sizes on disk.
172-183: Stronger validation for custom time format.Instead of checking dots, parse the suffix using
time.Parse(customFormat, suffix)to ensure correct formatting.Example:
name := files[0].Name() // test.log.YYYY-MM-DD... suffix := strings.TrimPrefix(name, "test.log.") _, perr := time.Parse(customFormat, strings.TrimSuffix(suffix, filepath.Ext(suffix))) require.NoError(t, perr)logfile/rotatingwriter.go (2)
167-175: Consistent error wrapping for rename/open failures.Wrap
os.RenameandopenFileerrors with context like you did elsewhere.Apply:
- err = os.Rename(rw.filePath, rw.rotatedFilePath()) - if err != nil { - return err - } + dst := rw.rotatedFilePath() + if err = os.Rename(rw.filePath, dst); err != nil { + return fmt.Errorf("error renaming rotating log file %q -> %q: %w", rw.filePath, dst, err) + } - file, size, err := openFile(rw.filePath, rw.filePerm) - if err != nil { - return err - } + file, size, err := openFile(rw.filePath, rw.filePerm) + if err != nil { + return fmt.Errorf("error reopening rotating log file %q: %w", rw.filePath, err) + }
214-217: Treat permission errors distinctly infileExists.Stat errors other than NotExist should be treated as “exists or unknown,” not “doesn’t exist.”
Apply:
-func fileExists(filePath string) bool { - _, err := os.Stat(filePath) - return err == nil -} +func fileExists(filePath string) bool { + _, err := os.Stat(filePath) + if err == nil { + return true + } + return !os.IsNotExist(err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)logfile/README.md(1 hunks)logfile/rotatingwriter.go(4 hunks)logfile/rotatingwriter_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
logfile/rotatingwriter_test.go (6)
logfile/rotatingwriter.go (1)
NewRotatingWriter(94-109)config.go (1)
NewConfig(43-56)jsonwriter.go (1)
NewJSONWriterConfig(23-35)textwriter.go (1)
NewTextWriterConfig(32-48)logger.go (1)
NewLogger(21-29)log/log.go (2)
Info(160-162)Flush(83-85)
🪛 markdownlint-cli2 (0.18.1)
logfile/README.md
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
logfile/rotatingwriter_test.go (1)
121-147: Comment vs behavior mismatch; consider clarifying rotation semantics.Your comments say “First write: 60 bytes, no rotation”, but with the current writer code (rotate before writing only when current size ≥ threshold), that holds. If you ever change logic to “rotate on crossing threshold,” this test’s expectation changes. Clarify intent in comments to avoid confusion.
logfile/rotatingwriter.go (1)
42-54: The review comment is incorrect.Since
go.modspecifiesgo 1.23, Go 1.22+ is already guaranteed. Usingcmp.Oris safe, idiomatic, and appropriate for the minimum supported version. The concern about supporting older Go versions does not apply here, and no refactoring is necessary.Likely an incorrect or invalid review comment.
| ``` | ||
| # Before rotation | ||
| myapp.log (10MB) | ||
| # After first rotation | ||
| myapp.log (0 bytes, new file) | ||
| myapp.log.2024-01-15_10:30:45 (10MB) | ||
| # After second rotation (same second) | ||
| myapp.log (0 bytes, new file) | ||
| myapp.log.2024-01-15_10:30:45 (10MB) | ||
| myapp.log.2024-01-15_10:30:45.1 (10MB) | ||
| ``` |
There was a problem hiding this comment.
Fix MD040: add language to fenced block.
Specify a language for the rotation example block to satisfy markdownlint.
Apply:
-```
+```text
# Before rotation
myapp.log (10MB)
# After first rotation
myapp.log (0 bytes, new file)
myapp.log.2024-01-15_10:30:45 (10MB)
# After second rotation (same second)
myapp.log (0 bytes, new file)
myapp.log.2024-01-15_10:30:45 (10MB)
myapp.log.2024-01-15_10:30:45.1 (10MB)
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In logfile/README.md around lines 171 to 183, the fenced code block showing the
rotation example lacks a language spec which triggers markdownlint rule MD040;
update the opening fence to include a language (for example "text") so it reads
| for i := range numWrites { | ||
| data := []byte(strings.Repeat("X", 60)) | ||
| _, err := writer.Write(data) | ||
| require.NoError(t, err, "write %d failed", i) | ||
| time.Sleep(10 * time.Millisecond) // Ensure different timestamps | ||
| } | ||
|
|
There was a problem hiding this comment.
Compilation error: range over int is invalid.
Replace for i := range N with classic for i := 0; i < N; i++ loops.
Apply:
- for i := range numWrites {
+ for i := 0; i < numWrites; i++ {
data := []byte(strings.Repeat("X", 60))
_, err := writer.Write(data)
require.NoError(t, err, "write %d failed", i)
time.Sleep(10 * time.Millisecond) // Ensure different timestamps
}
@@
- for i := range numGoroutines {
+ for i := 0; i < numGoroutines; i++ {
go func(id int) {
defer wg.Done()
- for j := range numWritesPerGoroutine {
+ for j := 0; j < numWritesPerGoroutine; j++ {
data := []byte(strings.Repeat("X", 30))
_, err := writer.Write(data)
assert.NoError(t, err, "goroutine %d, write %d failed", id, j)
}
}(i)
}
@@
- for i := range numThreads {
+ for i := 0; i < numThreads; i++ {
go func(thread int) {
- for threadMsg := range numThreadMessages {
+ for threadMsg := 0; threadMsg < numThreadMessages; threadMsg++ {
logger.Info("Thread log").
Int("thread", thread).
Int("threadMsg", threadMsg).
Str("filler", "Filler to over 100 bytes per log line in JSON and text").
Log()
time.Sleep(time.Millisecond)
}
wg.Done()
}(i)
}Also applies to: 280-289, 338-350, 340-347
🤖 Prompt for AI Agents
In logfile/rotatingwriter_test.go around lines 128-134 (and likewise at 280-289,
338-350, 340-347), the tests use "for i := range N" where N is an int, which
doesn't compile; replace each such loop with a standard indexed for loop "for i
:= 0; i < N; i++" so i iterates from 0 to N-1, keeping the loop body unchanged
and preserving any Sleep or assertions.
Summary by CodeRabbit