perf(logs): reduce market metrics log verbosity and duplication #223
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_cex_provider.dart (1)
136-143: Align defaults between interface and implementation; fix null‑safety on named params.Dart resolves default values at the static type. Having different defaults at the interface vs. implementation can yield divergent behavior. Also, non‑nullable named params without
required(e.g.,quotein the interface) are invalid under sound null‑safety.
- Make interface defaults explicit or mark params as
required.- Use the same default source (
CoinPaprikaIntervals.defaultInterval) in the implementation.Apply these diffs:
- Future<List<Ohlc>> fetchHistoricalOhlc({ + Future<List<Ohlc>> fetchHistoricalOhlc({ required String coinId, required DateTime startDate, DateTime? endDate, - QuoteCurrency quote, + required QuoteCurrency quote, String interval = CoinPaprikaIntervals.defaultInterval, });- Future<List<CoinPaprikaMarket>> fetchCoinMarkets({ + Future<List<CoinPaprikaMarket>> fetchCoinMarkets({ required String coinId, - List<QuoteCurrency> quotes, + List<QuoteCurrency> quotes = const [FiatCurrency.usd], });- Future<CoinPaprikaTicker> fetchCoinTicker({ + Future<CoinPaprikaTicker> fetchCoinTicker({ required String coinId, - List<QuoteCurrency> quotes, + List<QuoteCurrency> quotes = const [FiatCurrency.usd], });- QuoteCurrency quote = FiatCurrency.usd, - String interval = '1d', + QuoteCurrency quote = FiatCurrency.usd, + String interval = CoinPaprikaIntervals.defaultInterval,Also applies to: 167-170
🧹 Nitpick comments (19)
packages/komodo_cex_market_data/lib/src/repository_priority_manager.dart (1)
65-66: Make ordering deterministic when priorities tie (Dart List.sort is not stable).Add a secondary tie-breaker (e.g., type name) to avoid jitter across runs.
- final sorted = repositories.toList() - ..sort((a, b) => getPriority(a).compareTo(getPriority(b))); + final sorted = repositories.toList() + ..sort((a, b) { + final pa = getPriority(a), pb = getPriority(b); + final c = pa.compareTo(pb); + return c != 0 + ? c + : a.runtimeType.toString().compareTo(b.runtimeType.toString()); + });Do any callers rely on stable order for equal-priority repos? If yes, this change is necessary.
packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart (2)
79-84: OK to downgrade to fine; ensure aggregated visibility elsewhere.Since errors are swallowed here, confirm that higher-level warnings exist when all repos fail (they do in the fallback mixin).
90-94: Use the central sorter for consistency and to avoid comparator duplication.Prefer RepositoryPriorityManager.sortByPriority(candidates) over a manual comparator here.
packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (1)
93-97: Misleading message: CoinGecko path doesn’t actually fall back to configSymbol.Adjust wording to reflect behavior (no fallback ⇒ API cannot be used).
- _logger.fine( - 'Missing coinGeckoId for asset ${assetId.symbol.configSymbol}, ' - 'falling back to configSymbol. This may cause API issues.', - ); + _logger.fine( + 'Missing coinGeckoId for asset ${assetId.symbol.configSymbol}. ' + 'CoinGecko API cannot be used for this asset.', + );packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (1)
291-295: Type-mismatch in cache likely indicates corruption/migration—log at warning.This should be rare and actionable; keep it visible.
- _logger.fine( + _logger.warning( 'Cache entry for $symbol has unexpected type: ${raw.runtimeType}; ' 'Clearing entry and skipping', );packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart (2)
153-154: No healthy repos available—consider logging at info.This is an operationally relevant state; info level helps visibility without noise.
- _logger.fine('No healthy repositories available, using all repositories'); + _logger.info('No healthy repositories available, using all repositories');
334-337: End-of-retry exhaustion should be warning, not info.This represents a user-visible failure path.
- _logger.info( + _logger.warning( 'All $attemptCount attempts failed for $operationName ' '${assetId.symbol.assetConfigId}', );packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (5)
53-70: Preserve context when all endpoints fail.Throwing only the last exception loses endpoint context. Wrap with additional detail.
- throw lastException ?? Exception('All endpoints failed'); + if (lastException != null) { + throw Exception( + 'All endpoints failed for ${binanceApiEndpoint.join(", ")}. ' + 'Last error: $lastException', + ); + } + throw Exception('All endpoints failed for ${binanceApiEndpoint.join(", ")}');
95-100: Include both tickers in the validation error for easier debugging.- throw ArgumentError.value( - baseTicker, - 'baseTicker', - 'Base and rel coin tickers cannot be the same', - ); + throw ArgumentError.value( + baseTicker, + 'baseTicker', + 'Base and rel coin tickers cannot be the same ' + '(base: ${baseTicker.toUpperCase()}, rel: ${relTicker.toUpperCase()})', + );
147-152: Use identifier-style parameter name and add context in the message.- throw ArgumentError.value( - fiatCurrency, - 'Fiat Currency', - 'Coin and fiat coin cannot be the same', - ); + throw ArgumentError.value( + fiatCurrency, + 'fiatCurrency', + 'Coin and fiat coin cannot be the same ' + '(tradingSymbol: ${tradingSymbol.toUpperCase()}, ' + 'fiat: ${fiatCurrencyId.toUpperCase()})', + );
179-182: Minor: unify error message style across methods.Consider mirroring the contextual message used above here too.
- throw ArgumentError.value( - tradingSymbol, - 'tradingSymbol', - 'Coin and fiat coin cannot be the same', - ); + throw ArgumentError.value( + tradingSymbol, + 'tradingSymbol', + 'Coin and fiat coin cannot be the same ' + '(tradingSymbol: ${tradingSymbol.toUpperCase()}, ' + 'fiat: ${fiatCurrencyId.toUpperCase()})', + );
239-243: Same consistency nit: include both symbols in the message.- throw ArgumentError.value( - tradingSymbol, - 'tradingSymbol', - 'Coin and fiat coin cannot be the same', - ); + throw ArgumentError.value( + tradingSymbol, + 'tradingSymbol', + 'Coin and fiat coin cannot be the same ' + '(tradingSymbol: ${tradingSymbol.toUpperCase()}, ' + 'fiat: ${fiatCurrencyId.toUpperCase()})', + );packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_cex_provider.dart (4)
111-114: Add lightweight retries for transient failures (use maxRetries).All HTTP GETs now fail-fast on timeouts/network hiccups. Consider a small, capped retry with jitter for idempotent GETs to reduce flakes.
Apply this diff to reuse a helper:
- final response = await _httpClient - .get(uri, headers: _createRequestHeaderMap()) - .timeout(CoinPaprikaConfig.timeout); + final response = await _getWithRetry(uri);Add this helper (outside the shown range):
Future<http.Response> _getWithRetry(Uri uri) async { final headers = _createRequestHeaderMap(); Duration backoff = const Duration(milliseconds: 200); for (var attempt = 0;; attempt++) { try { return await _httpClient .get(uri, headers: headers) .timeout(CoinPaprikaConfig.timeout); } on TimeoutException catch (e) { if (attempt >= CoinPaprikaConfig.maxRetries) rethrow; } on http.ClientException catch (_) { if (attempt >= CoinPaprikaConfig.maxRetries) rethrow; } await Future<void>.delayed(backoff + Duration(milliseconds: attempt * 50)); backoff *= 2; } }
167-170: Deduplicate request logic across endpoints.Use the same retrying helper here for consistency and less boilerplate.
Apply this diff:
- final response = await _httpClient - .get(uri, headers: _createRequestHeaderMap()) - .timeout(CoinPaprikaConfig.timeout); + final response = await _getWithRetry(uri);Also applies to: 203-206, 239-242
330-345: Set JSON Accept header by default for GETs.Improves content negotiation and clarity. Safe, low-noise.
Map<String, String>? _createRequestHeaderMap({String? contentType}) { Map<String, String>? headers; if (contentType != null) { headers = <String, String>{ 'Content-Type': contentType, - 'Accept': contentType, + 'Accept': contentType, }; } if (_apiKey != null && _apiKey.isNotEmpty) { headers ??= <String, String>{}; headers['Authorization'] = 'Bearer $_apiKey'; } - return headers; + // Default Accept for reads when not otherwise specified. + (headers ??= <String, String>{})['Accept'] ??= 'application/json'; + return headers; }
23-25: Dead config constant (maxRetries) is unused.Either wire it into the new retry helper or drop it to avoid drift.
packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_repository.dart (3)
204-209: Prefer an assert over throwing for impossible condition.
actualBatchDurationis derived frommin(batchDuration, remaining); the > check is defensive but can be anassertto avoid user‑visible failures.- if (actualBatchDuration > batchDuration) { - throw ArgumentError.value( - actualBatchDuration, - 'actualBatchDuration', - 'Batch duration ${actualBatchDuration.inDays} days ' - 'exceeds safe limit of ${batchDuration.inDays} days', - ); - } + assert( + actualBatchDuration <= batchDuration, + 'Batch duration ${actualBatchDuration.inDays}d exceeds ' + 'limit ${batchDuration.inDays}d', + );
277-298: UTC‑normalizepriceDatefor historical price reads.Prevents missing data around DST/time‑zone shifts.
- if (priceDate != null) { + if (priceDate != null) { + final priceDateUtc = priceDate.toUtc(); // For historical prices, use OHLC data - final endDate = priceDate.add(const Duration(hours: 1)); + final endDate = priceDateUtc.add(const Duration(hours: 1)); final ohlcData = await getCoinOhlc( assetId, fiatCurrency, GraphInterval.oneHour, - startAt: priceDate, + startAt: priceDateUtc, endAt: endDate, );
322-325: UTC‑normalize date list and day boundaries when matching OHLC.Avoids day misalignment for non‑UTC input dates.
- final sortedDates = List<DateTime>.from(dates)..sort(); - final startDate = sortedDates.first.subtract(const Duration(hours: 1)); - final endDate = sortedDates.last.add(const Duration(hours: 1)); + final sortedDatesUtc = dates.map((d) => d.toUtc()).toList()..sort(); + final startDate = sortedDatesUtc.first.subtract(const Duration(hours: 1)); + final endDate = sortedDatesUtc.last.add(const Duration(hours: 1));- for (final date in dates) { - final dayStart = DateTime.utc(date.year, date.month, date.day); - final dayEnd = dayStart.add(const Duration(days: 1)).toUtc(); + for (final date in dates) { + final dateUtc = date.toUtc(); + final dayStart = DateTime.utc(dateUtc.year, dateUtc.month, dateUtc.day); + final dayEnd = dayStart.add(const Duration(days: 1));Also applies to: 326-329, 330-336, 338-364
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart(5 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart(0 hunks)packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_cex_provider.dart(8 hunks)packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_repository.dart(5 hunks)packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart(3 hunks)packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart(5 hunks)packages/komodo_cex_market_data/lib/src/repository_priority_manager.dart(1 hunks)packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/sparkline_repository.dart(3 hunks)
💤 Files with no reviewable changes (1)
- packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
- GitHub Check: Flutter tests (all packages)
🔇 Additional comments (10)
packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (2)
39-43: LGTM: quieter log level for missing binanceId.
146-150: LGTM: clearer, quieter log for missing coinPaprikaId.packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (2)
253-255: Fine-level “all repos failed” is okay if a higher-level warning exists.Given tryRepositoriesInOrderMaybe() logs a warning on total failure, this downgrade is acceptable.
Confirm the warning path is enabled in production logging filters.
311-311: Good downgrade to warning for cache read errors (recoverable).packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart (3)
112-115: Quieter failure-count log is fine; telemetry should capture rates separately.
300-304: Success-after-retry to fine: OK.
363-367: LGTM: keep final failure in Maybe() at warning.packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_cex_provider.dart (1)
266-271: Nice: clearer ArgumentError.value usage with parameter names.Improves DX and error traceability.
Also applies to: 285-293, 297-305, 314-320, 475-480
packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_repository.dart (2)
54-78: LGTM: simpler coin list path and cache priming.Filtering
isActiveand caching supported quotes upfront is clean. One nit: confirmCexCoin.currenciesexpects upper‑case codes; you currently pass mixed case then upper‑case only for the cache.Would you like me to standardize the
currenciesset to upper‑case as well for consistency?
371-388: LGTM: 24h change via ticker path.Straightforward and matches provider behavior.
packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_repository.dart
Show resolved
Hide resolved
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 8. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull Request Overview
This PR reduces log verbosity in the market data system by downgrading log levels from info/warning to fine for non-critical events and removing redundant error handling with try-catch blocks. The changes focus on reducing log noise while maintaining important error information.
- Downgraded many informational and warning logs to
finelevel for routine operations - Removed extensive try-catch blocks and their associated error logging from API providers
- Consolidated code style improvements including better use of cascade operators and ArgumentError constructors
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sparkline_repository.dart | Downgraded cache-related warning logs to fine level |
| repository_selection_strategy.dart | Reduced log level for repository support check failures |
| repository_priority_manager.dart | Minor code style improvement using cascade operator |
| repository_fallback_mixin.dart | Mixed log level adjustments and removed redundant log messages |
| id_resolution_strategy.dart | Downgraded missing ID warnings to fine level |
| coinpaprika_repository.dart | Removed extensive try-catch blocks and success logging |
| coinpaprika_cex_provider.dart | Removed try-catch blocks and improved ArgumentError usage |
| coingecko_cex_provider.dart | Removed redundant error logging |
| binance_repository.dart | Removed try-catch block and improved ArgumentError usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart
Outdated
Show resolved
Hide resolved
packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart
Outdated
Show resolved
Hide resolved
packages/komodo_cex_market_data/lib/src/coinpaprika/data/coinpaprika_repository.dart
Show resolved
Hide resolved
.infoand above to.fine,Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores