Agent-Hellboy / Agent-Hellboy/gunicorn-prometheus-exporter

Redundant Redis Storage Implementation in PR #82 - Should be Discarded

Offen
#92 1 Kommentar 0 Reaktionen 1 zugewiesene Person Beansprucht von @Agent-Hellboy Auf GitHub ansehen
good first issue
Vorherrschende Sprache
Python
Sterne
18
Forks
1
PR-Merge-Kennzahlen
Keine gemergten PRs in 30 T.

Beschreibung

## Problem

PR #82 contains a diff that adds a new file `src/gunicorn_prometheus_exporter/storage/redis_backend/storage_dict.py` with a `RedisDict` class. However, this implementation is **redundant** and **inferior** to the existing Redis storage functionality already present in the current codebase.

## Current Superior Implementation

The codebase already has a production-ready `RedisStorageDict` class in:
`src/gunicorn_prometheus_exporter/backend/core/client.py`

## Critical Issues in Proposed Implementation

### 1. **Blocking Redis Operations** 🚨
- Uses `redis.keys()` which blocks Redis server
- Current implementation uses `scan_iter()` (non-blocking)

### 2. **Type Handling Bugs** 🐛
- Attempts string operations on bytes objects
- Missing proper bytes-to-string conversion
- Will cause TypeError in production

### 3. **Missing Enterprise Features** ❌
- No TTL (Time-To-Live) support → Redis memory will grow indefinitely
- No process isolation → worker metrics will conflict
- No error handling → Redis failures will crash the application
- No logging/monitoring → silent failures
- No cleanup capabilities → dead worker metrics accumulate forever

## Comparison

| Feature | Current Implementation | Proposed Diff | Risk Level |
|---------|----------------------|---------------|------------|
| Redis Performance | ✅ Non-blocking `scan_iter` | ❌ Blocking `keys()` | **CRITICAL** |
| Memory Management | ✅ TTL support | ❌ No TTL | **HIGH** |
| Multi-process Safety | ✅ PID isolation | ❌ Shared namespace | **HIGH** |
| Error Handling | ✅ Graceful degradation | ❌ Hard crashes | **CRITICAL** |
| Process Cleanup | ✅ Automated cleanup | ❌ Manual only | **HIGH** |

## Recommendation

**Discard the entire diff for `storage/redis_backend/storage_dict.py`** because:

1. It duplicates existing functionality
2. It introduces production-breaking bugs
3. It uses an outdated file structure
4. The current implementation is already superior and production-tested

## Links

- **Current PR**: #82
- **Existing Implementation**: `src/gunicorn_prometheus_exporter/backend/core/client.py` (lines ~80-270)
- **PR Review Comment**: https://github.com/Agent-Hellboy/gunicorn-prometheus-exporter/pull/82#discussion_r2350338369

## Action Items

- [ ] Remove the proposed `storage/redis_backend/storage_dict.py` file from PR #82
- [ ] Keep the existing `backend/core/client.py` implementation
- [ ] Update any references to use the current backend structure
- [ ] Consider adding tests for the existing implementation if missing

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.