csxj2026/junhong_cmp_fiber

Files

2025-11-11 15:16:38 +08:00

8.1 KiB

Raw Blame History

Optional Improvements for 001-fiber-middleware-integration

Created: 2025-11-11
Status: Deferred - Address after core implementation
Source: /speckit.analyze findings (Medium/Low priority)

Medium Priority Improvements

A2: Log Retention Policy Clarity

Issue: FR-006 mentions "configured retention policy" but doesn't specify units/format
Current Impact: Implementation will need to infer format

Recommendation: Add to spec.md FR-006:

Retention policy specified in days (integer), e.g., 30 for app logs, 90 for access logs.
Implemented via Lumberjack MaxAge parameter.

Effort: 10 minutes (documentation only)

A3: Rate Limiting Default Values

Issue: FR-018a says "configurable requests per time window" but no default values
Current Impact: Developers must guess initial values

Recommendation: Add to spec.md FR-018a:

Default: 100 requests per minute per IP
Supported time units: second (s), minute (m), hour (h)
Example config: max=100, window=1m

Effort: 15 minutes (spec update + config.yaml example)

A5: Phase 0 Research Status

Issue: plan.md Phase 0 lists "Best Redis client" as unknown but Technical Context already decided
Current Impact: Confusion about whether research is needed
Recommendation: Update plan.md Phase 0:
- Remove "Best Redis client" from unknowns (already decided: go-redis/redis/v8)
- Or clarify: "Validate go-redis/redis/v8 choice (performance benchmarks, connection pool tuning)"
Effort: 5 minutes (documentation clarity)

A6: Config Validation Rules Location

Issue: T009 says "config loading with validation" but validation rules not documented
Current Impact: Developer must design validation rules during implementation

Recommendation: Create section in data-model.md or spec.md:

## Configuration Validation Rules
- server.port: 1024-65535 (int)
- server.host: non-empty string
- redis.addr: host:port format
- logging.max_size: 1-1000 (MB)
- logging.max_age: 1-365 (days)

Effort: 30 minutes (requires design decisions)

U2: Access Log Format Specification

Issue: FR-011 mentions logging HTTP requests but doesn't specify JSON schema for access.log
Current Impact: Access log structure determined during implementation

Recommendation: Add to spec.md or data-model.md:

{
  "timestamp": "2025-11-10T15:30:45Z",
  "level": "info",
  "request_id": "550e8400-e29b-...",
  "method": "GET",
  "path": "/api/v1/users",
  "status": 200,
  "duration_ms": 45,
  "ip": "192.168.1.100",
  "user_agent": "Mozilla/5.0...",
  "user_id": "12345" // if authenticated
}

Effort: 20 minutes (design + documentation)

U3: Panic Response Details

Issue: FR-014 says "HTTP 500 with unified error response" but unclear about stack trace handling
Current Impact: Security concern - should stack traces be in production responses?

Recommendation: Clarify in spec.md FR-014:

Development/Staging: Include sanitized error message in response.msg, full stack trace in logs only
Production: Generic error message "Internal server error", full details in logs only
Response format: {"code": 1000, "data": null, "msg": "Internal server error"}

Effort: 15 minutes (security consideration + spec update)

U4: Rate Limiter Documentation Format

Issue: FR-020 mentions "documentation" but unclear where (quickstart.md? inline comments? separate docs/)
Current Impact: Documentation may be incomplete or scattered

Recommendation: Specify in spec.md FR-020:

Documentation location: quickstart.md section "Enabling Rate Limiting"
Must include: Configuration parameters, per-endpoint setup, testing examples
Code example: Uncomment middleware, adjust max/window, test with curl loop

Effort: 10 minutes (clarify requirements)

U5: Non-Writable Log Directory Behavior

Issue: Edge case says "fail to start with clear error" but no HTTP status code if started
Current Impact: Unclear behavior if directory becomes non-writable at runtime

Recommendation: Clarify in spec.md Edge Cases:

Startup: Fail immediately with exit code 1 and error message before listening on port
Runtime: If directory becomes non-writable, log error to stderr and return 503 on health check

Effort: 15 minutes (design decision + spec update)

U6: Performance Test Task Missing

Issue: plan.md mentions "1000+ req/s capacity" but no performance test task in tasks.md
Current Impact: Performance goal not validated

Recommendation: Add task to tasks.md Phase 10:

- [ ] T117a [P] Load test with 1000 req/s for 60s, verify P95 < 200ms (use hey or wrk)

Effort: 1 hour (implementation + infrastructure setup)

I1: Logger Instances Documentation

Issue: spec.md mentions Zap but tasks reference appLogger/accessLogger instances
Current Impact: Spec doesn't explicitly require two logger instances

Recommendation: Add to spec.md FR-004 or Key Entities:

System maintains two independent Zap logger instances:
- appLogger: For application-level logs (business logic, errors, debug)
- accessLogger: For HTTP access logs (request/response details)
Each instance has separate Lumberjack rotation configuration.

Effort: 10 minutes (documentation clarity)

I3: Fail-Closed Timing Validation

Issue: spec.md FR-016b says "immediately" but T063 doesn't validate timing (< 100ms)
Current Impact: "Immediate" is subjective, no performance assertion
Recommendation: Update tasks.md T063:
```
- [ ] T063 [P] [US6] Unit test for Redis unavailable (fail closed with < 100ms response time)
```
Or clarify spec.md: "immediately = same request cycle, no retry delays"
Effort: 5 minutes (clarify requirements OR 30 minutes add timing assertion)

Low Priority Improvements

D1: Response Format Duplication

Issue: FR-007 and US3 both define unified response format
Current Impact: Redundancy, potential inconsistency if one updated

Recommendation: Keep FR-007 as normative, update US3 acceptance criteria:

Change: "the response contains `{...}`"
To: "the response follows unified format defined in FR-007"

Effort: 5 minutes (reduce duplication)

D2: Code Quality Task Consolidation

Issue: T096-T099 are four separate tasks that could run in one script
Current Impact: Overhead running tasks sequentially
Recommendation: Consider combining to T096-combined:
```
- [ ] T096 [P] Run code quality checks: gofmt -l ., go vet ./..., golangci-lint run, check doc comments
```
Or keep separate for granular progress tracking (current approach is also valid)
Effort: 15 minutes (script creation) OR keep as-is (no change needed)

I4: Task Count Verification

Issue: plan.md says 126 tasks, tasks.md has T001-T126 (now T127 with T012a)
Current Impact: None - counts match after update
Recommendation: No action needed, already addressed in C1 fix
Effort: 0 minutes (already resolved)

Priority Recommendation

Before Implementation:

None (all HIGH priority issues already fixed)

During Implementation (if time permits):

A6: Config validation rules (needed for T009 implementation)
U2: Access log format (needed for T050 implementation)
U3: Panic response details (security consideration)

After Implementation (polish phase):

A2, A3: Add default values to config.yaml and documentation
U4, U5, I1: Documentation improvements
U6: Performance testing (if infrastructure available)
D1, D2, I3: Nice-to-have optimizations

Summary

Total Optional Improvements: 14 items
Medium Priority: 9 items (mostly documentation/specification clarity)
Low Priority: 5 items (minor optimizations, already acceptable as-is)
Estimated Total Effort: ~4 hours for all medium priority items
Recommended Approach: Address medium priority items during implementation when context is fresh

8.1 KiB Raw Blame History