Skip to content

Commit a885261

Browse files
TexasCodingclaude
andcommitted
fix(stats): Address PR review feedback and fix all typing/linting issues
## Changes Made ### High Priority Fixes (PR Review) - Added comprehensive exception handling to all aggregator methods with graceful degradation - Enhanced PII sanitization for trading-specific data (accounts, balances, PnL, positions) - Smart redaction shows last 4 chars for account IDs - Shows positive/negative/zero for financial values - Extended sensitive key list for trading context ### Medium Priority Fixes (PR Review) - Added bounds checking to health score calculation (0-100 range enforced) - Fixed potential division by zero errors with safe fallbacks - Added comprehensive unit test suite covering: - Memory leak prevention with circular buffers - PII sanitization verification - Thread safety under concurrent access - Performance percentile calculations - Error handling and graceful degradation ### Typing and Linting Fixes - Fixed OrderManager response type checking with isinstance() - Fixed EnhancedStatsTrackingMixin type annotations and data_quality handling - Fixed StatisticsAggregator ComponentStats usage (dict literals instead of constructor) - Fixed TradingSuiteStats missing fields (total_errors, health_score) - Fixed managed_trade.py null checking for order objects - Fixed position_manager operations authentication method calls - Removed unused imports and trailing whitespace ### Files Modified - src/project_x_py/order_manager/core.py - src/project_x_py/position_manager/operations.py - src/project_x_py/risk_manager/managed_trade.py - src/project_x_py/trading_suite.py - src/project_x_py/types/stats_types.py - src/project_x_py/utils/enhanced_stats_tracking.py - src/project_x_py/utils/statistics_aggregator.py - tests/test_enhanced_statistics.py (new comprehensive test suite) All mypy type checking and ruff linting now pass with zero issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 3d4e80c commit a885261

File tree

8 files changed

+769
-151
lines changed

8 files changed

+769
-151
lines changed

src/project_x_py/order_manager/core.py

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,17 @@ async def place_order(
454454
)
455455
duration_ms = (time.time() - start_time) * 1000
456456

457+
# Response should be a dict for order placement
458+
if not isinstance(response, dict):
459+
error = ProjectXOrderError("Invalid response format")
460+
await self.track_error(
461+
error, "place_order", {"contract_id": contract_id, "side": side}
462+
)
463+
await self.track_operation(
464+
"place_order", duration_ms, success=False
465+
)
466+
raise error
467+
457468
if not response.get("success", False):
458469
error_msg = response.get("errorMessage", ErrorMessages.ORDER_FAILED)
459470
error = ProjectXOrderError(error_msg)
@@ -465,7 +476,12 @@ async def place_order(
465476
)
466477
raise error
467478

468-
result = OrderPlaceResponse(**response)
479+
result = OrderPlaceResponse(
480+
orderId=response.get("orderId", 0),
481+
success=response.get("success", False),
482+
errorCode=response.get("errorCode", ""),
483+
errorMessage=response.get("errorMessage", ""),
484+
)
469485

470486
# Track successful operation
471487
await self.track_operation(
@@ -481,7 +497,9 @@ async def place_order(
481497
self.stats["total_volume"] += size
482498
if size > self.stats["largest_order"]:
483499
self.stats["largest_order"] = size
484-
self._update_activity()
500+
self._last_activity = (
501+
datetime.now()
502+
) # Update activity timestamp directly
485503

486504
self.logger.info(
487505
LogMessages.ORDER_PLACED,
@@ -544,6 +562,10 @@ async def search_open_orders(
544562
"POST", "/Order/searchOpen", data=params
545563
)
546564

565+
# Response should be a dict for order search
566+
if not isinstance(response, dict):
567+
raise ProjectXOrderError("Invalid response format")
568+
547569
if not response.get("success", False):
548570
error_msg = response.get("errorMessage", ErrorMessages.ORDER_SEARCH_FAILED)
549571
raise ProjectXOrderError(error_msg)
@@ -665,6 +687,10 @@ async def cancel_order(self, order_id: int, account_id: int | None = None) -> bo
665687
"POST", "/Order/cancel", data=payload
666688
)
667689

690+
# Response should be a dict
691+
if not isinstance(response, dict):
692+
raise ProjectXOrderError("Invalid response format")
693+
668694
success = response.get("success", False) if response else False
669695

670696
if success:
@@ -763,6 +789,10 @@ async def modify_order(
763789
"POST", "/Order/modify", data=payload
764790
)
765791

792+
# Response should be a dict
793+
if not isinstance(response, dict):
794+
raise ProjectXOrderError("Invalid response format")
795+
766796
if response and response.get("success", False):
767797
# Update statistics
768798
async with self.order_lock:
@@ -866,7 +896,7 @@ async def get_order_statistics(self) -> OrderManagerStats:
866896
"""
867897
# Get enhanced performance metrics
868898
perf_metrics = await self.get_performance_metrics()
869-
error_stats = await self.get_error_stats()
899+
# error_stats = await self.get_error_stats() # Available if needed
870900

871901
async with self.order_lock:
872902
# Use internal order tracking

src/project_x_py/position_manager/operations.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,9 @@ async def close_position_direct(
126126
- No price control - executes at current market price
127127
- For partial closes, use partially_close_position()
128128
"""
129-
await self.project_x._ensure_authenticated()
129+
# Ensure authenticated (using public method, not private _ensure_authenticated)
130+
if not self.project_x.account_info:
131+
await self.project_x.authenticate()
130132

131133
if account_id is None:
132134
if not self.project_x.account_info:
@@ -250,7 +252,9 @@ async def partially_close_position(
250252
- Remaining position continues with same average price
251253
- Close size must not exceed current position size
252254
"""
253-
await self.project_x._ensure_authenticated()
255+
# Ensure authenticated (using public method, not private _ensure_authenticated)
256+
if not self.project_x.account_info:
257+
await self.project_x.authenticate()
254258

255259
if account_id is None:
256260
if not self.project_x.account_info:

src/project_x_py/risk_manager/managed_trade.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,7 +641,7 @@ async def order_fill_handler(event: Any) -> None:
641641
event_order_id = event_data.get("order_id")
642642
if not event_order_id and "order" in event_data:
643643
order_obj = event_data.get("order")
644-
if hasattr(order_obj, "id"):
644+
if order_obj and hasattr(order_obj, "id"):
645645
event_order_id = order_obj.id
646646
if event_order_id == order.id:
647647
filled_successfully = True
@@ -656,7 +656,7 @@ async def order_terminal_handler(event: Any) -> None:
656656
event_order_id = event_data.get("order_id")
657657
if not event_order_id and "order" in event_data:
658658
order_obj = event_data.get("order")
659-
if hasattr(order_obj, "id"):
659+
if order_obj and hasattr(order_obj, "id"):
660660
event_order_id = order_obj.id
661661
if event_order_id == order.id:
662662
filled_successfully = False

src/project_x_py/trading_suite.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
PositionManagerConfig,
6262
)
6363
from project_x_py.types.protocols import ProjectXClientProtocol
64-
from project_x_py.types.stats_types import ComponentStats, TradingSuiteStats
64+
from project_x_py.types.stats_types import TradingSuiteStats
6565
from project_x_py.utils import ProjectXLogger, StatisticsAggregator
6666

6767
logger = ProjectXLogger.get_logger(__name__)

src/project_x_py/types/stats_types.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ class ComponentStats(TypedDict):
7676
last_activity: str | None
7777
error_count: int
7878
memory_usage_mb: float
79+
performance_metrics: NotRequired[dict[str, Any]] # Optional performance data
7980

8081

8182
class TradingSuiteStats(TypedDict):
@@ -112,6 +113,10 @@ class TradingSuiteStats(TypedDict):
112113
features_enabled: list[str]
113114
timeframes: list[str]
114115

116+
# Calculated cross-component metrics
117+
total_errors: NotRequired[int] # Total error count across all components
118+
health_score: NotRequired[float] # Overall health score (0-100)
119+
115120

116121
# Component-Specific Statistics Types
117122
class OrderManagerStats(TypedDict):

src/project_x_py/utils/enhanced_stats_tracking.py

Lines changed: 86 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import traceback
2626
from collections import deque
2727
from datetime import datetime, timedelta
28-
from typing import Any, Callable, Optional
28+
from typing import Any
2929

3030
from project_x_py.utils.logging_config import ProjectXLogger
3131

@@ -87,7 +87,7 @@ def _init_enhanced_stats(
8787
}
8888

8989
# Data quality metrics
90-
self._data_quality = {
90+
self._data_quality: dict[str, Any] = {
9191
"total_data_points": 0,
9292
"invalid_data_points": 0,
9393
"missing_data_points": 0,
@@ -211,10 +211,18 @@ async def track_data_quality(
211211
duplicate_points: Number of duplicate points
212212
"""
213213
async with self._stats_lock:
214-
self._data_quality["total_data_points"] += total_points
215-
self._data_quality["invalid_data_points"] += invalid_points
216-
self._data_quality["missing_data_points"] += missing_points
217-
self._data_quality["duplicate_data_points"] += duplicate_points
214+
# Type-safe integer updates
215+
current_total = int(self._data_quality.get("total_data_points", 0))
216+
current_invalid = int(self._data_quality.get("invalid_data_points", 0))
217+
current_missing = int(self._data_quality.get("missing_data_points", 0))
218+
current_duplicate = int(self._data_quality.get("duplicate_data_points", 0))
219+
220+
self._data_quality["total_data_points"] = current_total + total_points
221+
self._data_quality["invalid_data_points"] = current_invalid + invalid_points
222+
self._data_quality["missing_data_points"] = current_missing + missing_points
223+
self._data_quality["duplicate_data_points"] = (
224+
current_duplicate + duplicate_points
225+
)
218226
self._data_quality["last_validation"] = datetime.now()
219227

220228
async def get_performance_metrics(self) -> dict[str, Any]:
@@ -316,8 +324,8 @@ async def get_data_quality_stats(self) -> dict[str, Any]:
316324
Dictionary with data quality metrics
317325
"""
318326
async with self._stats_lock:
319-
total = self._data_quality["total_data_points"]
320-
invalid = self._data_quality["invalid_data_points"]
327+
total = int(self._data_quality.get("total_data_points", 0))
328+
invalid = int(self._data_quality.get("invalid_data_points", 0))
321329

322330
quality_score = ((total - invalid) / total * 100) if total > 0 else 100.0
323331

@@ -424,11 +432,14 @@ async def cleanup_old_stats(self) -> None:
424432

425433
# Clean up data gaps
426434
if "data_gaps" in self._data_quality:
427-
self._data_quality["data_gaps"] = [
428-
gap
429-
for gap in self._data_quality["data_gaps"]
430-
if gap.get("timestamp", datetime.min) >= cutoff_time
431-
]
435+
gaps = self._data_quality.get("data_gaps", [])
436+
if isinstance(gaps, list):
437+
self._data_quality["data_gaps"] = [
438+
gap
439+
for gap in gaps
440+
if isinstance(gap, dict)
441+
and gap.get("timestamp", datetime.min) >= cutoff_time
442+
]
432443

433444
logger.debug(f"Cleaned up stats older than {cutoff_time}")
434445

@@ -481,7 +492,7 @@ def _calculate_memory_usage(self) -> float:
481492
size += sys.getsizeof(attr)
482493

483494
# For collections, also count items
484-
if isinstance(attr, (list, dict, set, deque)):
495+
if isinstance(attr, list | dict | set | deque):
485496
try:
486497
for item in attr.values() if isinstance(attr, dict) else attr:
487498
size += sys.getsizeof(item)
@@ -525,24 +536,81 @@ def _sanitize_for_export(self, data: Any) -> Any:
525536
"""
526537
if isinstance(data, dict):
527538
sanitized = {}
539+
# Extended list of sensitive keys for trading data
528540
sensitive_keys = {
529541
"password",
530542
"token",
531543
"key",
532544
"secret",
533545
"auth",
534546
"credential",
547+
"account_id",
548+
"accountid",
549+
"account_name",
550+
"accountname",
551+
"balance",
552+
"equity",
553+
"pnl",
554+
"profit",
555+
"loss",
556+
"position_size",
557+
"positionsize",
558+
"order_size",
559+
"ordersize",
560+
"api_key",
561+
"apikey",
562+
"session",
563+
"cookie",
564+
"username",
565+
"email",
566+
"phone",
567+
"ssn",
568+
"tax_id",
569+
"bank",
570+
"routing",
535571
}
536572

537573
for key, value in data.items():
538-
if any(sensitive in key.lower() for sensitive in sensitive_keys):
539-
sanitized[key] = "***REDACTED***"
574+
key_lower = key.lower()
575+
# Check if key contains any sensitive patterns
576+
if any(sensitive in key_lower for sensitive in sensitive_keys):
577+
# Special handling for certain fields to show partial info
578+
if (
579+
"account" in key_lower
580+
and isinstance(value, str)
581+
and len(value) > 4
582+
):
583+
# Show last 4 chars of account ID/name
584+
sanitized[key] = f"***{value[-4:]}"
585+
elif any(
586+
x in key_lower
587+
for x in ["pnl", "profit", "loss", "balance", "equity"]
588+
):
589+
# Show if positive/negative but not actual value
590+
if isinstance(value, int | float):
591+
sanitized[key] = (
592+
"positive"
593+
if value > 0
594+
else "negative"
595+
if value < 0
596+
else "zero"
597+
)
598+
else:
599+
sanitized[key] = "***REDACTED***"
600+
else:
601+
sanitized[key] = "***REDACTED***"
540602
else:
541603
sanitized[key] = self._sanitize_for_export(value)
542604

543605
return sanitized
544-
elif isinstance(data, (list, tuple)):
606+
elif isinstance(data, list | tuple):
545607
return [self._sanitize_for_export(item) for item in data]
608+
elif isinstance(data, str):
609+
# Check for patterns that look like sensitive data
610+
if len(data) > 20 and any(c in data for c in ["=", "Bearer", "Basic"]):
611+
# Might be a token or auth header
612+
return "***REDACTED***"
613+
return data
546614
else:
547615
return data
548616

@@ -562,7 +630,7 @@ def _format_prometheus(self, stats: dict[str, Any]) -> str:
562630
# Performance metrics
563631
if "performance" in stats:
564632
perf = stats["performance"]
565-
if "api_stats" in perf and perf["api_stats"]:
633+
if perf.get("api_stats"):
566634
lines.append(
567635
f"# HELP {component}_api_response_time_ms API response time in milliseconds"
568636
)

0 commit comments

Comments
 (0)