added logging to the service#9
Merged
Merged
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 6
🧹 Nitpick comments (1)
am-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowLogger.java (1)
124-133: ⚡ Quick winFail fast on odd key/value argument counts.
At Line 128, odd-length
kvinputs are silently truncated. Prefer explicit validation to catch bad call sites early.💡 Suggested fix
private static Map<String, String> toMap(Object... kv) { Map<String, String> out = new LinkedHashMap<>(); if (kv == null) return out; + if ((kv.length % 2) != 0) { + throw new IllegalArgumentException("Key/value arguments must be in pairs"); + } for (int i = 0; i + 1 < kv.length; i += 2) { if (kv[i] != null) { out.put(String.valueOf(kv[i]), kv[i + 1] == null ? "null" : String.valueOf(kv[i + 1])); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowLogger.java` around lines 124 - 133, The toMap method in FlowLogger silently truncates odd-length kv arrays; add an explicit validation at the start of toMap(Object... kv) that if kv != null and kv.length is odd it throws an IllegalArgumentException (with a clear message mentioning the method and expected key/value pairs) so callers fail fast; preserve the existing behavior for kv == null (return empty map) and otherwise continue building the LinkedHashMap as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.java`:
- Around line 637-685: The controller builds the LTP response from
marketDataCacheService.getLivePrices(...) but ignores an "error" payload; update
the method that constructs the LTP response to check livePrices for an "error"
key (or a failure flag) immediately after calling
marketDataCacheService.getLivePrices (inspect the livePrices Map and the
pricesObj), and if present return a propagated error ResponseEntity (with the
original error body and an appropriate HTTP status instead of continuing to
build the empty result). Ensure you reference
marketDataCacheService.getLivePrices, the livePrices Map, pricesObj and the
result-building block so the new early-return replaces the current fall-through
behavior.
- Around line 216-239: The POST handlers in MarketDataController call
TimeFrame.fromApiValue(...) and treat any exception as a 500, causing invalid
timeFrame inputs to return INTERNAL_ERROR; change the handlers to explicitly
validate the TimeFrame conversion (e.g., catch the specific exception thrown by
TimeFrame.fromApiValue, such as IllegalArgumentException or a custom parse
exception) and return a 400 Bad Request with an error payload (error/message)
and flowLogger.fail(span, e) for that case instead of the generic 500 path;
apply the same change to the other POST handler block that also calls
TimeFrame.fromApiValue (the second block referenced in the comment) so malformed
client input consistently yields 400.
- Around line 397-406: The controller currently uses a shared SimpleDateFormat
instance (used in getOptionChain and getMutualFundNavHistory) which is not
thread-safe and is lenient; replace that usage with java.time APIs: remove the
shared SimpleDateFormat field and parse expiryDate and navDate using
DateTimeFormatter with ResolverStyle.STRICT (e.g.,
DateTimeFormatter.ISO_LOCAL_DATE or a custom formatter) and LocalDate.parse,
catch DateTimeParseException instead of ParseException, log the invalid date and
return the same 400 response payload; alternatively, if you keep a formatter
object make it an immutable static DateTimeFormatter to ensure thread-safety and
strict parsing.
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/service/impl/MarketDataFetchServiceImpl.java`:
- Around line 88-94: The code in MarketDataFetchServiceImpl dereferences the
result of marketDataService.getOHLC(...) (ohlcData.size()) without a null check;
change the logic to guard against a null return by treating null as an empty
map: if ohlcData is null, replace it with Collections.emptyMap() (or new
HashMap<>()), then put "quotes" with that safe map, "count" as its size, and
"cached" as !forceRefresh; update the block around the call to
marketDataService.getOHLC(...) to normalize nulls before using ohlcData.
- Around line 29-30: The class MarketDataFetchServiceImpl still uses legacy
two-argument logger calls like log.info(methodName, msg) from the `@Slf4j`
migration; locate every log.*(...) in MarketDataFetchServiceImpl where the first
argument is the methodName variable (the occurrences noted around lines
referenced in the review) and remove that extra argument so SLF4J receives a
proper message/format and optional placeholders (e.g., change
log.info(methodName, msg) to log.info(msg) or log.info("... {} ...", arg) as
appropriate); ensure error/warn/debug calls are updated consistently and, when
including context, convert the methodName into part of the formatted message or
use placeholders rather than a separate first argument.
In
`@am-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowSpan.java`:
- Around line 45-49: addAttribute(String key, Object value) writes new entries
into MDC and initialFields but close() only restores keys from priorMdc so
attributes added by the span remain in MDC after close; update close() (method
close in class FlowSpan) to remove or restore any keys that were added by
addAttribute (i.e., keys present in initialFields but not in priorMdc) and
ensure priorMdc entries are put back into MDC, so the MDC is fully restored to
its prior state when FlowSpan is closed.
---
Nitpick comments:
In
`@am-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowLogger.java`:
- Around line 124-133: The toMap method in FlowLogger silently truncates
odd-length kv arrays; add an explicit validation at the start of toMap(Object...
kv) that if kv != null and kv.length is odd it throws an
IllegalArgumentException (with a clear message mentioning the method and
expected key/value pairs) so callers fail fast; preserve the existing behavior
for kv == null (return empty map) and otherwise continue building the
LinkedHashMap as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70a8c1d9-979d-49c3-8f91-f4cf3e715803
📒 Files selected for processing (19)
am-market-data/market-data-analysis/src/main/java/com/am/marketdata/analysis/controller/AnalysisController.javaam-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.javaam-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/SecurityController.javaam-market-data/market-data-api/src/main/java/com/am/marketdata/api/filter/ApiLoggingFilter.javaam-market-data/market-data-api/src/main/java/com/am/marketdata/api/service/impl/MarketDataFetchServiceImpl.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/config/ObservabilityConfig.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowLogger.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowMarkers.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/FlowSpan.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/MdcKeys.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/observability/MdcTaskDecorator.javaam-market-data/market-data-common/src/main/java/com/am/marketdata/common/util/LoggingSanitizer.javaam-market-data/market-data-kafka/src/main/java/com/am/marketdata/kafka/producer/BaseKafkaProducer.javaam-market-data/market-data-provider/src/main/java/com/am/marketdata/provider/upstox/UpstoxApiService.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/config/ThreadPoolConfig.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/service/MarketDataService.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/service/util/HistoricalDataRetriever.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/service/util/OHLCDataRetriever.javaam-market-data/market-data-watchlist/src/main/java/com/am/marketdata/watchlist/controller/WatchlistController.java
Comment on lines
+216
to
239
| Map<String, Object> quotesResponse = marketDataCacheService.getQuotes( | ||
| symbolList, request.isIndexSymbol(), TimeFrame.fromApiValue(request.getTimeFrame()), | ||
| request.isForceRefresh()); | ||
|
|
||
| // Use cache service instead of direct service call | ||
| Map<String, Object> quotesResponse = marketDataCacheService.getQuotes( | ||
| symbolList, request.isIndexSymbol(), TimeFrame.fromApiValue(request.getTimeFrame()), | ||
| request.isForceRefresh()); | ||
| if (quotesResponse.containsKey("ERROR")) { | ||
| log.warn("Quotes fetch post returned error branch symbolsCount={}", symbolList.size()); | ||
| Map<String, Object> errorResponse = new HashMap<>(); | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> errorDetails = (Map<String, Object>) quotesResponse.get("ERROR"); | ||
| errorResponse.put("error", errorDetails.get("error")); | ||
| errorResponse.put("message", errorDetails.get("message")); | ||
| flowLogger.fail(span, new Exception(String.valueOf(errorDetails.get("message")))); | ||
| return ResponseEntity.internalServerError().body(errorResponse); | ||
| } | ||
|
|
||
| // Check if there was an error | ||
| if (quotesResponse.containsKey("ERROR")) { | ||
| flowLogger.complete(span, "resultCount", quotesResponse.size()); | ||
| return ResponseEntity.ok(quotesResponse); | ||
| } catch (Exception e) { | ||
| log.error("Error processing quotes post request symbolsCount={}", symbolList.size(), e); | ||
| Map<String, Object> errorResponse = new HashMap<>(); | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> errorDetails = (Map<String, Object>) quotesResponse.get("ERROR"); | ||
| errorResponse.put("error", errorDetails.get("error")); | ||
| errorResponse.put("message", errorDetails.get("message")); | ||
| errorResponse.put("error", "INTERNAL_ERROR"); | ||
| errorResponse.put("message", e.getMessage()); | ||
| flowLogger.fail(span, e); | ||
| return ResponseEntity.internalServerError().body(errorResponse); |
There was a problem hiding this comment.
Return 400 for invalid timeFrame in the POST handlers.
Both methods call TimeFrame.fromApiValue(...) under the generic Exception catch, so malformed client input now comes back as 500. That regresses the API contract and the GET /quotes behavior, which already treats this as request validation.
Also applies to: 263-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.java`
around lines 216 - 239, The POST handlers in MarketDataController call
TimeFrame.fromApiValue(...) and treat any exception as a 500, causing invalid
timeFrame inputs to return INTERNAL_ERROR; change the handlers to explicitly
validate the TimeFrame conversion (e.g., catch the specific exception thrown by
TimeFrame.fromApiValue, such as IllegalArgumentException or a custom parse
exception) and return a 400 Bad Request with an error payload (error/message)
and flowLogger.fail(span, e) for that case instead of the generic 500 path;
apply the same change to the other POST handler block that also calls
TimeFrame.fromApiValue (the second block referenced in the comment) so malformed
client input consistently yields 400.
Comment on lines
+397
to
+406
| try { | ||
| expiry = dateFormat.parse(expiryDate); | ||
| } catch (ParseException e) { | ||
| log.warn("Invalid date format for option chain underlying={} expiryDate={}", underlyingSymbol, | ||
| expiryDate); | ||
| Map<String, Object> errorResponse = new HashMap<>(); | ||
| errorResponse.put("error", "Invalid date format"); | ||
| errorResponse.put("message", "Use yyyy-MM-dd format for expiry date"); | ||
| flowLogger.warn(span, "Invalid date format: " + expiryDate); | ||
| return ResponseEntity.badRequest().body(errorResponse); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is java.text.SimpleDateFormat thread-safe when shared across a singleton Spring bean, and is it lenient by default for invalid dates like 2026-02-31?
💡 Result:
No, java.text.SimpleDateFormat is not thread-safe when shared across a singleton Spring bean. The official Java documentation states that "Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally."[1][2] Multiple sources confirm this, noting that SimpleDateFormat mutates internal state (like its Calendar field) during parse and format operations, leading to race conditions and incorrect results in multi-threaded environments like Spring singletons (default scope for beans).[3][4][5][6] Yes, it is lenient by default for invalid dates like 2026-02-31. The DateFormat superclass documentation (inherited by SimpleDateFormat) explicitly states: "By default, parsing is lenient: If the input is not in the form used by this object's format method but can still be parsed as a date, then the parse succeeds."[2] For example, with pattern "yyyy-MM-dd", "2026-02-31" would parse to March 3, 2026 (rolling over the invalid day), rather than throwing an exception. Strict parsing requires calling setLenient(false).[7][8][9] Recommendations: Use java.time.format.DateTimeFormatter (Java 8+) instead, which is thread-safe and immutable. Or use ThreadLocal for thread confinement.[3]
Citations:
- 1: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/text/SimpleDateFormat.html
- 2: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/text/DateFormat.html
- 3: https://www.baeldung.com/java-simple-date-format
- 4: https://stackoverflow.com/questions/6840803/why-is-javas-simpledateformat-not-thread-safe
- 5: https://stackoverflow.com/questions/5652518/confusion-about-thread-safety-simpledateformat-example
- 6: https://www.callicoder.com/java-simpledateformat-thread-safety-issues/
- 7: https://stackoverflow.com/questions/7606387/what-is-the-use-of-lenient
- 8: https://howtodoinjava.com/java/date-time/strict-date-validation-simpledateformat-setlenient/
- 9: https://stackoverflow.com/questions/50746771/simpledateformat-leniency-leads-to-unexpected-behavior
🏁 Script executed:
fd -t f "MarketDataController.java" --exec wc -l {} \;Repository: AM-Portfolio/am-market
Length of output: 176
🏁 Script executed:
head -80 ./am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.javaRepository: AM-Portfolio/am-market
Length of output: 3445
🏁 Script executed:
sed -n '390,420p' ./am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.javaRepository: AM-Portfolio/am-market
Length of output: 1891
🏁 Script executed:
sed -n '500,530p' ./am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.javaRepository: AM-Portfolio/am-market
Length of output: 1801
Replace SimpleDateFormat with thread-safe, strict date parsing.
This controller uses a shared SimpleDateFormat field that is not thread-safe in a singleton Spring bean. Additionally, the formatter is lenient by default—invalid dates like 2026-02-31 will parse to 2026-03-03 instead of raising ParseException, bypassing the 400 validation branches. Use java.time.format.DateTimeFormatter (thread-safe and immutable) or disable leniency with setLenient(false).
Affects both getOptionChain (lines 397–406) and getMutualFundNavHistory (lines 507–516).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.java`
around lines 397 - 406, The controller currently uses a shared SimpleDateFormat
instance (used in getOptionChain and getMutualFundNavHistory) which is not
thread-safe and is lenient; replace that usage with java.time APIs: remove the
shared SimpleDateFormat field and parse expiryDate and navDate using
DateTimeFormatter with ResolverStyle.STRICT (e.g.,
DateTimeFormatter.ISO_LOCAL_DATE or a custom formatter) and LocalDate.parse,
catch DateTimeParseException instead of ParseException, log the invalid date and
return the same 400 response payload; alternatively, if you keep a formatter
object make it an immutable static DateTimeFormatter to ensure thread-safety and
strict parsing.
Comment on lines
+637
to
+685
| Map<String, OHLCQuote> historicalData = marketDataCacheService.getOHLC(symbolList, indexSymbol, tf, false); | ||
| Map<String, Object> livePrices = marketDataCacheService.getLivePrices(symbolList, indexSymbol, | ||
| forceRefresh); | ||
|
|
||
| Map<String, Map<String, Object>> result = new HashMap<>(); | ||
| Object pricesObj = livePrices.get("prices"); | ||
| if (pricesObj instanceof List) { | ||
| @SuppressWarnings("unchecked") | ||
| List<Map<String, Object>> pricesList = (List<Map<String, Object>>) pricesObj; | ||
|
|
||
| for (Map<String, Object> priceData : pricesList) { | ||
| String symbol = (String) priceData.get("symbol"); | ||
| if (symbol == null) | ||
| continue; | ||
|
|
||
| Double currentPrice = ((Number) priceData.get("lastPrice")).doubleValue(); | ||
| OHLCQuote historical = historicalData.get(symbol); | ||
| if (historical == null) { | ||
| historical = historicalData.get("NSE:" + symbol); | ||
| } | ||
|
|
||
| double previousClose = 0.0; | ||
| if (historical != null && historical.getOhlc() != null) { | ||
| previousClose = historical.getOhlc().getClose(); | ||
| } | ||
|
|
||
| double change = currentPrice - previousClose; | ||
| double changePercent = previousClose != 0 ? (change / previousClose) * 100 : 0.0; | ||
|
|
||
| Map<String, Object> ltpData = new HashMap<>(); | ||
| ltpData.put("symbol", symbol); | ||
| ltpData.put("lastPrice", currentPrice); | ||
| ltpData.put("previousClose", previousClose); | ||
| ltpData.put("change", change); | ||
| ltpData.put("changePercent", changePercent); | ||
| ltpData.put("timeframe", tf.getApiValue()); | ||
|
|
||
| result.put(symbol, ltpData); | ||
| } | ||
|
|
||
| // Calculate change and percentage | ||
| double change = currentPrice - previousClose; | ||
| double changePercent = previousClose != 0 ? (change / previousClose) * 100 : 0.0; | ||
|
|
||
| Map<String, Object> ltpData = new HashMap<>(); | ||
| ltpData.put("symbol", symbol); | ||
| ltpData.put("lastPrice", currentPrice); | ||
| ltpData.put("previousClose", previousClose); | ||
| ltpData.put("change", change); | ||
| ltpData.put("changePercent", changePercent); | ||
| ltpData.put("timeframe", tf.getApiValue()); | ||
|
|
||
| result.put(symbol, ltpData); | ||
| } | ||
| } | ||
|
|
||
| Map<String, Object> response = new HashMap<>(); | ||
| response.put("count", result.size()); | ||
| response.put("timeframe", tf.getApiValue()); | ||
| response.put("data", result); | ||
| response.put("timestamp", new Date()); | ||
|
|
||
| log.info("getLiveLTP", "Successfully calculated LTP with change for " + result.size() + " symbols"); | ||
| return ResponseEntity.ok(response); | ||
| Map<String, Object> response = new HashMap<>(); | ||
| response.put("count", result.size()); | ||
| response.put("timeframe", tf.getApiValue()); | ||
| response.put("data", result); | ||
| response.put("timestamp", new Date()); | ||
|
|
||
| } catch (Exception e) { | ||
| log.error("getLiveLTP", "Unexpected error while fetching live LTP: " + e.getMessage(), e); | ||
| Map<String, Object> errorResponse = new HashMap<>(); | ||
| errorResponse.put("error", "Failed to fetch live LTP"); | ||
| errorResponse.put("message", e.getMessage()); | ||
| return ResponseEntity.internalServerError().body(errorResponse); | ||
| flowLogger.complete(span, "resultCount", result.size()); | ||
| return ResponseEntity.ok(response); |
There was a problem hiding this comment.
Propagate getLivePrices(...) errors before building the LTP response.
marketDataCacheService.getLivePrices(...) already uses an "error" payload for failures, but this method only inspects "prices". If the downstream call fails, the controller currently falls through to count = 0 and returns 200, which hides the real error from clients and monitoring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/controller/MarketDataController.java`
around lines 637 - 685, The controller builds the LTP response from
marketDataCacheService.getLivePrices(...) but ignores an "error" payload; update
the method that constructs the LTP response to check livePrices for an "error"
key (or a failure flag) immediately after calling
marketDataCacheService.getLivePrices (inspect the livePrices Map and the
pricesObj), and if present return a propagated error ResponseEntity (with the
original error body and an appropriate HTTP status instead of continuing to
build the empty result). Ensure you reference
marketDataCacheService.getLivePrices, the livePrices Map, pricesObj and the
result-building block so the new early-return replaces the current fall-through
behavior.
Comment on lines
+88
to
+94
| Map<String, OHLCQuote> ohlcData = marketDataService.getOHLC(new ArrayList<>(symbols), timeFrame, | ||
| forceRefresh, null); | ||
|
|
||
| Map<String, Object> response = new HashMap<>(); | ||
| response.put("quotes", ohlcData); | ||
| response.put("count", ohlcData.size()); | ||
| response.put("cached", !forceRefresh); |
There was a problem hiding this comment.
Handle null OHLC responses before dereferencing.
At Line 93-94, ohlcData.size() is called without a null guard. If marketDataService.getOHLC(...) returns null, this path throws NPE.
💡 Suggested fix
Map<String, OHLCQuote> ohlcData = marketDataService.getOHLC(new ArrayList<>(symbols), timeFrame,
forceRefresh, null);
+ if (ohlcData == null) {
+ ohlcData = Collections.emptyMap();
+ }
Map<String, Object> response = new HashMap<>();
response.put("quotes", ohlcData);
response.put("count", ohlcData.size());📝 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.
Suggested change
| Map<String, OHLCQuote> ohlcData = marketDataService.getOHLC(new ArrayList<>(symbols), timeFrame, | |
| forceRefresh, null); | |
| Map<String, Object> response = new HashMap<>(); | |
| response.put("quotes", ohlcData); | |
| response.put("count", ohlcData.size()); | |
| response.put("cached", !forceRefresh); | |
| Map<String, OHLCQuote> ohlcData = marketDataService.getOHLC(new ArrayList<>(symbols), timeFrame, | |
| forceRefresh, null); | |
| if (ohlcData == null) { | |
| ohlcData = Collections.emptyMap(); | |
| } | |
| Map<String, Object> response = new HashMap<>(); | |
| response.put("quotes", ohlcData); | |
| response.put("count", ohlcData.size()); | |
| response.put("cached", !forceRefresh); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-api/src/main/java/com/am/marketdata/api/service/impl/MarketDataFetchServiceImpl.java`
around lines 88 - 94, The code in MarketDataFetchServiceImpl dereferences the
result of marketDataService.getOHLC(...) (ohlcData.size()) without a null check;
change the logic to guard against a null return by treating null as an empty
map: if ohlcData is null, replace it with Collections.emptyMap() (or new
HashMap<>()), then put "quotes" with that safe map, "count" as its size, and
"cached" as !forceRefresh; update the block around the call to
marketDataService.getOHLC(...) to normalize nulls before using ohlcData.
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
temporarily deployed
to
preprod-am-market-data
GitHub Actions
Inactive
SagaRoy12
had a problem deploying
to
prod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
temporarily deployed
to
preprod-am-market-data
GitHub Actions
Inactive
SagaRoy12
had a problem deploying
to
prod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
…l duplicate logic
…eys, MdcTaskDecorator, ApiLoggingFilter)
…ry API compatibility)
…Logger.warn with log.warn
SagaRoy12
had a problem deploying
to
dev-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
dev-am-market-data
GitHub Actions
Failure
- Add callerData provider to logback-spring.xml (class, method, file, line) - Restore tool.name field to match shared observability library schema - Update application.yml comments for accuracy - Maintains the fix for the Kubernetes CrashLoopBackOff
SagaRoy12
had a problem deploying
to
dev-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
preprod-am-market-data
GitHub Actions
Failure
SagaRoy12
had a problem deploying
to
dev-am-market-data
GitHub Actions
Failure
SagaRoy12
temporarily deployed
to
preprod-am-market-data
GitHub Actions
Inactive
SagaRoy12
had a problem deploying
to
prod-am-market-data
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
am-market-data/market-data-app/src/main/resources/application.yml (1)
67-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand request-log sanitization for additional credential key names.
Line 69–74 still misses common keys already used in this codebase (for example
api-key,api_key,secret-key,password,authorization). With request logging enabled, those can leak.Suggested hardening
sanitize: # Mask sensitive fields in log payloads (PII/Credentials) extra-fields: + - authorization + - password + - passwd + - secret + - secret-key + - api_key + - api-key - code - request_token - access_token - refresh_token - client_secret🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-market-data/market-data-app/src/main/resources/application.yml` around lines 67 - 74, The sanitize.extra-fields list currently masks only a few credential keys (e.g. code, request_token, access_token); expand this list in application.yml under the sanitize → extra-fields section to include common credential key names used across the codebase such as api-key, api_key, apiKey, secret-key, secret_key, secretKey, password, authorization (and any header variants like Authorization-Bearer if used) so request logging cannot leak these values; update the extra-fields array in place where sanitize is defined to include these additional keys.
🧹 Nitpick comments (1)
am-market-data/market-data-app/src/main/resources/application.yml (1)
236-237: ⚡ Quick winUse a safer default trace sampling rate.
Line 237 defaults to
1.0(sample everything). IfTRACING_SAMPLING_PROBABILITYis missing in a non-local env, trace volume/cost can spike unexpectedly. Consider a lower fallback (for example0.1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-market-data/market-data-app/src/main/resources/application.yml` around lines 236 - 237, The trace sampling default currently sets sampling.probability to 1.0 via ${TRACING_SAMPLING_PROBABILITY:1.0}, which causes full sampling when the env var is missing; change the fallback to a safer lower value (e.g., 0.1) so sampling.probability uses ${TRACING_SAMPLING_PROBABILITY:0.1} instead, updating the configuration entry named "sampling" and its "probability" key to prevent unexpected trace volume/cost spikes in non-local environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@am-market-data/market-data-app/src/main/resources/logback-spring.xml`:
- Line 48: Remove the <mdc/> provider entry so Logback stops emitting every MDC
key; instead rely on the explicit whitelisted pattern fields already defined
(the pattern entries declared around the existing whitelist block). Locate the
<mdc/> element in the logback-spring.xml configuration and delete that element,
leaving only the configured %X{...} pattern fields (the whitelist) in the
encoder/layout so MDC keys are included only via explicit %X entries.
---
Duplicate comments:
In `@am-market-data/market-data-app/src/main/resources/application.yml`:
- Around line 67-74: The sanitize.extra-fields list currently masks only a few
credential keys (e.g. code, request_token, access_token); expand this list in
application.yml under the sanitize → extra-fields section to include common
credential key names used across the codebase such as api-key, api_key, apiKey,
secret-key, secret_key, secretKey, password, authorization (and any header
variants like Authorization-Bearer if used) so request logging cannot leak these
values; update the extra-fields array in place where sanitize is defined to
include these additional keys.
---
Nitpick comments:
In `@am-market-data/market-data-app/src/main/resources/application.yml`:
- Around line 236-237: The trace sampling default currently sets
sampling.probability to 1.0 via ${TRACING_SAMPLING_PROBABILITY:1.0}, which
causes full sampling when the env var is missing; change the fallback to a safer
lower value (e.g., 0.1) so sampling.probability uses
${TRACING_SAMPLING_PROBABILITY:0.1} instead, updating the configuration entry
named "sampling" and its "probability" key to prevent unexpected trace
volume/cost spikes in non-local environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2de7f8fc-ba38-4402-844b-ee34c5cf141b
📒 Files selected for processing (3)
am-market-data/market-data-app/pom.xmlam-market-data/market-data-app/src/main/resources/application.ymlam-market-data/market-data-app/src/main/resources/logback-spring.xml
✅ Files skipped from review due to trivial changes (1)
- am-market-data/market-data-app/pom.xml
| </pattern> | ||
| <message/> | ||
| <loggerName/> | ||
| <mdc/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether sensitive-looking keys are written into MDC anywhere in the repo.
rg -nP --type=java -C2 '\bMDC\.put\(\s*"([^"]+)"'
rg -nP --type=java -C2 -i '\bMDC\.put\(\s*"(authorization|password|secret|token|api[_-]?key|cookie|session|jwt)'Repository: AM-Portfolio/am-market
Length of output: 983
🏁 Script executed:
# Read the logback-spring.xml file to understand the full context around line 48
cat -n am-market-data/market-data-app/src/main/resources/logback-spring.xml | head -60Repository: AM-Portfolio/am-market
Length of output: 3348
🏁 Script executed:
# Search more comprehensively for all MDC.put calls to ensure we have the complete picture
rg -n 'MDC\.put' --type=javaRepository: AM-Portfolio/am-market
Length of output: 4224
Remove the <mdc/> provider to enforce explicit MDC field whitelisting.
The <mdc/> provider on line 48 emits every MDC entry, which defeats the explicit field control established in lines 31–42. This creates an unintended exposure vector if future code adds sensitive context to MDC. Keep only the whitelisted pattern fields.
Suggested change
- <mdc/>
<arguments/>📝 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.
Suggested change
| <mdc/> | |
| <arguments/> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-market-data/market-data-app/src/main/resources/logback-spring.xml` at line
48, Remove the <mdc/> provider entry so Logback stops emitting every MDC key;
instead rely on the explicit whitelisted pattern fields already defined (the
pattern entries declared around the existing whitelist block). Locate the <mdc/>
element in the logback-spring.xml configuration and delete that element, leaving
only the configured %X{...} pattern fields (the whitelist) in the encoder/layout
so MDC keys are included only via explicit %X entries.
… and OTel tracing - Replaced all raw stdout `print(...)` statements across all API and background service layers with standard python logger statements. - Upgraded exception handlings to use `logger.error(..., exc_info=True)` for full structured stack trace capturing. - Hardened `logger_config.py` JSON formatter to prevent Mapped Diagnostic Context (MDC) parameters from overwriting active OpenTelemetry tracing IDs. - Configured legacy `AMLogger` and `LoggerMixin` to dynamically generate and bind unique `traceId` and `spanId` fallbacks to MDC when running outside an active OTel span context. - Bound critical transaction context tags (like `job_id`, `file_id`, `sheet_id`, `parse_method`, and `symbol`) to MDC scope blocks. - Added three comprehensive verification unit tests to validate fallback wrappers, client mixins, and OTel tracing priority constraints.
…am-parser/am_api/api.py
…el in am-parser - Relocate FastAPIInstrumentor and HTTPXClientInstrumentor out of the lifespan hook and into the module-level of api.py. - Ensure correct ASGI middleware wrapping order so that the OpenTelemetry middleware correctly wraps the RequestLoggingASGIMiddleware. - Add real-time OTel trace context diagnostics inside send_wrapper to verify traceId/spanId presence during request execution. - Fixes traceId and spanId timing propagation issues when running under production Uvicorn workers in Kubernetes.
…s for stock indices
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
am-parser/am_api/api.py (1)
751-755:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve HTTPException status codes in
upload_file.Line 644 raises
HTTPException(400)for invalid file type, but Lines 751-755 wrap all exceptions into 500.Suggested fix
- except Exception as e: + except HTTPException: + raise + except Exception as e: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Failed to complete upload workflow: {str(e)}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-parser/am_api/api.py` around lines 751 - 755, The catch-all except block in upload_file currently converts every exception into an HTTP 500; change it to detect and re-raise existing HTTPException instances (i.e., if isinstance(e, HTTPException): raise) and only wrap non-HTTPException errors into the HTTPException with status 500 and the detail message, so original 4xx/other statuses (like the earlier HTTPException(400) for invalid file type) are preserved.am-parser/am_api/etf_api.py (2)
50-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep intentional HTTP errors and always close
ETFServiceinfetch_all_etf_holdings.This block currently converts raised
HTTPException(e.g., Line 65) into 500 and can skipawait etf_service.close()when errors occur before Line 105.Suggested fix
- except Exception as e: + except HTTPException: + raise + except Exception as e: logger.error("ETF holdings fetch error", exc_info=True) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Failed to start ETF holdings fetch: {str(e)}" ) + finally: + try: + await etf_service.close() + except Exception: + pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-parser/am_api/etf_api.py` around lines 50 - 133, The handler (fetch_all_etf_holdings) currently catches all exceptions and turns HTTPException into a 500 while also potentially skipping await etf_service.close(); change the flow so HTTPException is re-raised unchanged and ETFService is always closed: wrap the main logic in try/except/finally (or add a finally block) that awaits etf_service.close() if etf_service was created, and in the except block re-raise the exception when isinstance(e, HTTPException) before converting other exceptions to a 500; reference ETFService, await etf_service.close(), and HTTPException to locate the spots to modify.
298-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same exception/resource handling fix in
fetch_holdings_for_etf.This function has the same pattern: broad catch remaps intended client errors to 500 and
ETFServicemay remain unclosed on intermediate failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-parser/am_api/etf_api.py` around lines 298 - 377, The fetch_holdings_for_etf function should mirror the safer resource/error handling used elsewhere: ensure ETFService is always closed (use try/finally or a contextmanager around ETFService instance) so etf_service.close() runs on all paths, and avoid remapping intended HTTPExceptions to 500 by catching HTTPException and re-raising it before a broad except; keep a narrow top-level except for unexpected exceptions to log and return 500. Locate symbols: fetch_holdings_for_etf, ETFService, etf_service.close(), get_job_queue(), job_queue.create_job(), and the current broad except block — modify control flow so ETFService is closed in finally and HTTPException is re-raised rather than swallowed.am-parser/am_api/job_api.py (1)
36-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose
service_instanceinupload_excel_asyncto avoid connection leaks.The request path creates a fresh DB service (Line 38) but never closes it. Under load, this can accumulate open clients/connections.
Suggested fix
async def upload_excel_async( @@ - try: + service_instance = None + try: # Initialize services with proper database connection service_instance = create_mutual_fund_service( @@ except Exception as e: logger.error("Upload error", exc_info=True) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Failed to upload file: {str(e)}" ) + finally: + if service_instance: + await service_instance.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-parser/am_api/job_api.py` around lines 36 - 119, upload_excel_async creates a DB client via create_mutual_fund_service (service_instance) but never closes it; wrap the try/except or add a finally block to always shut down the client (call the appropriate teardown on service_instance such as service_instance.close(), service_instance.disconnect(), service_instance.shutdown() or await service_instance.close() if async) after using FileUploadRepository and job_queue so connections aren’t leaked; ensure the cleanup runs on both success and in the except branch and check for service_instance being not None before calling the teardown.
🧹 Nitpick comments (1)
am-parser/am_configs/settings.py (1)
75-82: ⚡ Quick winConstrain observability mode fields to fail fast on invalid env values.
On Line 75 and Line 81,
otel_traces_exporterandlog_formatare free-form strings; typos will be accepted and fail later in downstream setup. Consider constraining these (andlog_level) withLiteraltypes so misconfiguration fails at startup.Proposed diff
-from typing import Optional +from typing import Optional, Literal @@ - otel_traces_exporter: str = "otlp" + otel_traces_exporter: Literal["otlp"] = "otlp" @@ - log_level: str = "INFO" + log_level: Literal["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG"] = "INFO" @@ - log_format: str = "json" + log_format: Literal["json", "text"] = "json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@am-parser/am_configs/settings.py` around lines 75 - 82, The fields otel_traces_exporter, log_format, and log_level in am_configs/settings.py are currently plain strings and should be constrained to fail fast on invalid env values; change their type annotations to typing.Literal with the allowed values (e.g., otel_traces_exporter: Literal["otlp", "none", ...] and log_format: Literal["json","text"], log_level: Literal["DEBUG","INFO","WARNING","ERROR","CRITICAL"]) and import Literal from typing so Pydantic (or static checking) will validate and raise immediately on misconfiguration; update the field annotations for otel_traces_exporter, log_format, and log_level accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@am-market-data/market-data-redis/src/main/java/com/am/marketdata/redis/config/RedisConfig.java`:
- Around line 131-132: The file RedisConfig.java contains stray characters ("d
") that cause a parse/compile error; open the RedisConfig class and remove the
extraneous "d" (or any non-code characters) at the location shown so the Java
source is valid again—ensure the class declaration, imports, and methods (e.g.,
any constructor or methods inside RedisConfig) are intact and compile cleanly
after removing the stray characters.
In
`@am-market-data/market-data-scheduler/src/main/java/com/am/marketdata/scheduler/service/StreamerScheduler.java`:
- Around line 30-32: Update the outdated comments that still state a 9:00 AM
schedule to reflect the new 9:10 AM time: locate the StreamerScheduler class
comment and the Javadoc/comment above the executeStartStreaming method and
change their text to mention 9:10 AM (matching the `@Scheduled` cron "0 10 9 ? *
MON-FRI" and the log in executeStartStreaming) so comments and code are
consistent.
In
`@am-market-data/market-data-service/src/main/java/com/am/marketdata/service/MarketDataCacheService.java`:
- Around line 705-747: The loop in MarketDataCacheService currently has a single
try/catch around the entire quotes.entrySet() iteration so one malformed quote
or JSON error aborts caching for all symbols; move error handling inside the
loop by wrapping each entry's processing (the body that normalizes rawSymbol,
builds cacheData, calls objectMapper.writeValueAsString and
redisTemplate.opsForValue().set) in its own try/catch, log the exception with
the rawSymbol/symbol and continue to the next entry; apply the same per-entry
try/catch refactor to the other similar method referenced (the block around
lines 760-795) so individual symbol failures don’t stop the batch.
- Line 790: The map is being populated with the raw request key (`symbol`) which
can differ in casing/prefix from the normalized key (`cleanSymbol`) and cause
cache-join misses; update the insertion to use the normalized key by replacing
result.put(symbol, quote) with result.put(cleanSymbol, quote) (ensure
`cleanSymbol` is computed/available in the same scope and handle any null/empty
case consistently).
In `@am-parser/am_api/api.py`:
- Line 649: The log call uses an unnecessary f-string prefix on a static
message; update the logger.info invocation in am_api.api (the logger.info call
that passes extra={"parse_method": parse_method}) to use a plain string (remove
the leading "f") so it's not flagged as F541.
- Line 71: The logger currently prints settings.mongo_uri (exposing
credentials); update the log to avoid printing the full URI by parsing
settings.mongo_uri and logging only safe metadata (host(s), port(s), and
database name) instead of credentials—modify the logging call around
logger.info(f"Connecting to MongoDB: {settings.mongo_uri}") to extract and log
host/db info (using a URI parser or pymongo.uri_parser.parse_uri) and emit a
message like "Connecting to MongoDB host(s): ..., db: ...".
- Line 180: Remove the debug stdout print that emits span internals in the
middleware (the print call that starts with "[DEBUG_OTEL]" in
am_parser/am_api/api.py); either delete that print or replace it with a
structured logger call at debug level (e.g., using the existing request/logger
in the middleware) and guard it behind a debug flag so it does not flood stdout
on every response; ensure you reference and use the span_ctx/current_span
variables already present but do not write to raw stdout.
- Around line 652-669: The code dereferences global services
(file_upload_service, file_upload_repo, file_processing_service,
service_instance) without readiness checks, causing AttributeError on partial
startup; before calling save_uploaded_file, create_file_upload, or
process_excel_file (and before using bind_context) add explicit guards that
verify each global is non-None/ready and raise or return a 503 Service
Unavailable (or raise an appropriate HTTP/ServiceUnavailable exception) with a
clear log message; ensure you check file_upload_service before
save_uploaded_file/get_excel_sheet_info, file_upload_repo before
create_file_upload, and file_processing_service before process_excel_file,
logging the missing service name and short-circuiting rather than letting an
AttributeError propagate.
In `@am-parser/am_api/etf_api.py`:
- Line 70: Several logger calls use f-strings with no placeholders (Ruff F541);
remove the unnecessary f-prefix so plain strings are used. Locate the
logger.info/debug calls that currently read things like f"Starting async ETF
holdings fetch" (and the other messages at the positions flagged) and change
them to plain string literals (e.g., "Starting async ETF holdings fetch") while
preserving the same extra= and other arguments; update all occurrences
referenced (the logger.* calls at the lines flagged: 70, 320, 453, 463, 466,
469).
In `@am-parser/am_api/job_api.py`:
- Line 64: The logger.info call uses an unnecessary f-string prefix with no
placeholders (F541); update the call in job_api.py (the logger.info line that
currently reads logger.info(f"Excel split completed", extra={"sheet_count":
sheet_count})) by removing the leading f so it becomes logger.info("Excel split
completed", extra={"sheet_count": sheet_count}) to avoid the linter warning.
In `@am-parser/am_common/logging/core.py`:
- Line 80: The timestamp value currently builds with
datetime.datetime.now(datetime.timezone.utc).isoformat() + "Z", which produces
an invalid ISO-8601 string like "...+00:00Z"; fix by removing the explicit +"Z"
so the "timestamp" field uses
datetime.datetime.now(datetime.timezone.utc).isoformat() (or alternatively
replace the "+00:00" with "Z" via .replace('+00:00','Z') if you need Zulu
notation), updating the line that sets the "timestamp" key in the CLS payload in
core.py.
In `@am-parser/am_common/observability/tracing.py`:
- Around line 28-31: The code currently always creates a new TracerProvider,
sets it via trace.set_tracer_provider(...), and stores it in _TRACER_PROVIDER
which can detach processors if another initializer already registered a global
provider; change configure_tracing to first call trace.get_tracer_provider() and
use that existing provider if it's already a real provider (not the
default/no-op), only create and set a new TracerProvider if none exists, then
attach the BatchSpanProcessor/exporter to the chosen provider (use the symbols
provider, _TRACER_PROVIDER, trace.get_tracer_provider,
trace.set_tracer_provider, and BatchSpanProcessor so the processor is always
registered to the active global provider).
In `@am-parser/am_llm/together_service.py`:
- Line 77: The lint errors come from using f-strings with no interpolation in
logger.info calls; locate the logger.info calls in together_service.py (the one
that logs "Sheet dimensions" and the other two logger.info occurrences around
the later df/shape logs) and remove the leading f from those string literals
(e.g., change f"Sheet dimensions" to "Sheet dimensions") so only strings that
actually interpolate use the f-prefix; keep any f-strings that do include
placeholders intact.
In `@am-parser/am_services/file_processing_service.py`:
- Around line 191-194: The code currently sets per-request API keys in
process-global os.environ which risks leaking secrets and race conditions;
remove the os.environ mutation and instead thread the api_key parameter into the
parsing call chain (_parse_sheet_file and _sync_parse_file) and construct a
local TogetherLLMService using TogetherLLMService(api_key=api_key) where needed
(e.g., inside the file processing handler and any helper that uses the LLM) so
each request uses its own service instance without mutating global environment
state.
In `@am-parser/am_services/job_queue_service.py`:
- Line 103: The logger.info calls use unnecessary f-strings (e.g., the call that
passes extra={"stuck_count": len(stuck_jobs)}) which triggers F541; remove the
f-prefix so the message is a plain string (or use a proper formatted
placeholder) and keep the structured extra payload intact—update both
logger.info invocations in job_queue_service.py that mention stuck jobs (the one
using stuck_jobs/stuck_count and the similar persistent-stuck log) to use non-f
string literals.
In `@am-parser/tests/test_observability.py`:
- Around line 113-117: Tests in test_observability.py mutate global and named
logger state (e.g., root_logger = logging.getLogger(), removing handlers and
adding handler) without restoring it, causing cross-test pollution; fix by
wrapping these manipulations in a pytest fixture or try/finally that saves the
original logger state (handlers, level, propagate, and any named logger
handlers) before changes and restores them after the test, updating occurrences
around the root_logger variable and any places using handler,
logging.getLogger(), or specific named logger instances so each test resets to
the original handlers/levels on teardown.
---
Outside diff comments:
In `@am-parser/am_api/api.py`:
- Around line 751-755: The catch-all except block in upload_file currently
converts every exception into an HTTP 500; change it to detect and re-raise
existing HTTPException instances (i.e., if isinstance(e, HTTPException): raise)
and only wrap non-HTTPException errors into the HTTPException with status 500
and the detail message, so original 4xx/other statuses (like the earlier
HTTPException(400) for invalid file type) are preserved.
In `@am-parser/am_api/etf_api.py`:
- Around line 50-133: The handler (fetch_all_etf_holdings) currently catches all
exceptions and turns HTTPException into a 500 while also potentially skipping
await etf_service.close(); change the flow so HTTPException is re-raised
unchanged and ETFService is always closed: wrap the main logic in
try/except/finally (or add a finally block) that awaits etf_service.close() if
etf_service was created, and in the except block re-raise the exception when
isinstance(e, HTTPException) before converting other exceptions to a 500;
reference ETFService, await etf_service.close(), and HTTPException to locate the
spots to modify.
- Around line 298-377: The fetch_holdings_for_etf function should mirror the
safer resource/error handling used elsewhere: ensure ETFService is always closed
(use try/finally or a contextmanager around ETFService instance) so
etf_service.close() runs on all paths, and avoid remapping intended
HTTPExceptions to 500 by catching HTTPException and re-raising it before a broad
except; keep a narrow top-level except for unexpected exceptions to log and
return 500. Locate symbols: fetch_holdings_for_etf, ETFService,
etf_service.close(), get_job_queue(), job_queue.create_job(), and the current
broad except block — modify control flow so ETFService is closed in finally and
HTTPException is re-raised rather than swallowed.
In `@am-parser/am_api/job_api.py`:
- Around line 36-119: upload_excel_async creates a DB client via
create_mutual_fund_service (service_instance) but never closes it; wrap the
try/except or add a finally block to always shut down the client (call the
appropriate teardown on service_instance such as service_instance.close(),
service_instance.disconnect(), service_instance.shutdown() or await
service_instance.close() if async) after using FileUploadRepository and
job_queue so connections aren’t leaked; ensure the cleanup runs on both success
and in the except branch and check for service_instance being not None before
calling the teardown.
---
Nitpick comments:
In `@am-parser/am_configs/settings.py`:
- Around line 75-82: The fields otel_traces_exporter, log_format, and log_level
in am_configs/settings.py are currently plain strings and should be constrained
to fail fast on invalid env values; change their type annotations to
typing.Literal with the allowed values (e.g., otel_traces_exporter:
Literal["otlp", "none", ...] and log_format: Literal["json","text"], log_level:
Literal["DEBUG","INFO","WARNING","ERROR","CRITICAL"]) and import Literal from
typing so Pydantic (or static checking) will validate and raise immediately on
misconfiguration; update the field annotations for otel_traces_exporter,
log_format, and log_level accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db62fcc5-54b7-4349-be7b-a8ac32faf50b
📒 Files selected for processing (32)
.gitignoream-market-data/helm/values.dev.yamlam-market-data/market-data-analysis/src/main/java/com/am/marketdata/analysis/controller/AnalysisController.javaam-market-data/market-data-api/src/main/java/com/am/marketdata/api/service/StockIndicesService.javaam-market-data/market-data-redis/src/main/java/com/am/marketdata/redis/config/RedisConfig.javaam-market-data/market-data-scheduler/src/main/java/com/am/marketdata/scheduler/service/StreamerScheduler.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/service/MarketDataCacheService.javaam-market-data/market-data-service/src/main/java/com/am/marketdata/service/websocket/service/StreamerManager.javaam-parser/.envam-parser/.env.exampleam-parser/.env.templateam-parser/am_api/api.pyam-parser/am_api/etf_api.pyam-parser/am_api/job_api.pyam-parser/am_common/__init__.pyam-parser/am_common/logging/am_logging_client.pyam-parser/am_common/logging/core.pyam-parser/am_common/observability/__init__.pyam-parser/am_common/observability/config.pyam-parser/am_common/observability/context.pyam-parser/am_common/observability/logger_config.pyam-parser/am_common/observability/logging.pyam-parser/am_common/observability/tracing.pyam-parser/am_configs/settings.pyam-parser/am_llm/parser.pyam-parser/am_llm/together_service.pyam-parser/am_services/file_processing_service.pyam-parser/am_services/file_upload_service.pyam-parser/am_services/job_queue_service.pyam-parser/helm/values.dev.yamlam-parser/requirements.txtam-parser/tests/test_observability.py
💤 Files with no reviewable changes (3)
- am-parser/.env
- am-parser/.env.template
- am-parser/.env.example
✅ Files skipped from review due to trivial changes (5)
- am-parser/am_common/observability/logging.py
- .gitignore
- am-parser/requirements.txt
- am-parser/am_llm/parser.py
- am-parser/am_common/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- am-market-data/market-data-analysis/src/main/java/com/am/marketdata/analysis/controller/AnalysisController.java
Comment on lines
+131
to
+132
|
|
||
| d No newline at end of file |
There was a problem hiding this comment.
Remove the stray characters causing a parse error.
Line 132 contains invalid characters d that will prevent compilation.
🐛 Proposed fix
}
-
-d
+📝 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.
Suggested change
| d | |
| } | |
🧰 Tools
🪛 PMD (7.24.0)
[High] 132-132: Parse Error: ParseException: Parse exception in file 'am-market-data/market-data-redis/src/main/java/com/am/marketdata/redis/config/RedisConfig.java' at line 132, column 3: Encountered .
Was expecting one of:
...
"<" ...
(Parse Error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-redis/src/main/java/com/am/marketdata/redis/config/RedisConfig.java`
around lines 131 - 132, The file RedisConfig.java contains stray characters ("d
") that cause a parse/compile error; open the RedisConfig class and remove the
extraneous "d" (or any non-code characters) at the location shown so the Java
source is valid again—ensure the class declaration, imports, and methods (e.g.,
any constructor or methods inside RedisConfig) are intact and compile cleanly
after removing the stray characters.
Comment on lines
+30
to
+32
| @Scheduled(cron = "0 10 9 ? * MON-FRI", zone = "Asia/Kolkata") | ||
| public void executeStartStreaming() { | ||
| log.info("⏰ Triggering scheduled Streamer start at 9:00 AM..."); | ||
| log.info("⏰ Triggering scheduled Streamer start at 9:10 AM..."); |
There was a problem hiding this comment.
Update outdated comments to reflect the new 9:10 AM schedule.
The cron expression and log message correctly reference 9:10 AM, but the class-level comment (line 15) and method-level comment (line 27) still reference the old 9:00 AM schedule.
📝 Proposed fix
Update line 15:
- * Auto-start at 9:00 AM Monday-Friday (market open)
+ * Auto-start at 9:10 AM Monday-Friday (market open)Update line 27:
- * Start the streamer (Scheduled at 9:00 AM Monday-Friday)
+ * Start the streamer (Scheduled at 9:10 AM Monday-Friday)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-scheduler/src/main/java/com/am/marketdata/scheduler/service/StreamerScheduler.java`
around lines 30 - 32, Update the outdated comments that still state a 9:00 AM
schedule to reflect the new 9:10 AM time: locate the StreamerScheduler class
comment and the Javadoc/comment above the executeStartStreaming method and
change their text to mention 9:10 AM (matching the `@Scheduled` cron "0 10 9 ? *
MON-FRI" and the log in executeStartStreaming) so comments and code are
consistent.
Comment on lines
+705
to
+747
| try { | ||
| for (Map.Entry<String, OHLCQuote> entry : quotes.entrySet()) { | ||
| String rawSymbol = entry.getKey(); | ||
| // Normalize symbol: e.g. NSE_INDEX|Nifty 50 -> NIFTY 50 | ||
| String symbol = rawSymbol; | ||
| if (symbol.contains("|")) { | ||
| symbol = symbol.substring(symbol.indexOf("|") + 1); | ||
| } | ||
| symbol = symbol.toUpperCase().trim(); | ||
|
|
||
| OHLCQuote quote = entry.getValue(); | ||
| if (quote == null) { | ||
| continue; | ||
| } | ||
|
|
||
| double lastPrice = quote.getLastPrice(); | ||
| double open = quote.getOhlc() != null ? quote.getOhlc().getOpen() : 0.0; | ||
| double high = quote.getOhlc() != null ? quote.getOhlc().getHigh() : 0.0; | ||
| double low = quote.getOhlc() != null ? quote.getOhlc().getLow() : 0.0; | ||
| double previousClose = quote.getPreviousClose(); | ||
| double change = lastPrice - previousClose; | ||
| double changePercent = previousClose != 0 ? (change / previousClose) * 100.0 : 0.0; | ||
|
|
||
| Map<String, Object> cacheData = new HashMap<>(); | ||
| cacheData.put("symbol", symbol); | ||
| cacheData.put("lastPrice", lastPrice); | ||
| cacheData.put("open", open); | ||
| cacheData.put("high", high); | ||
| cacheData.put("low", low); | ||
| cacheData.put("previousClose", previousClose); | ||
| cacheData.put("change", change); | ||
| cacheData.put("changePercent", changePercent); | ||
| cacheData.put("updatedAt", System.currentTimeMillis()); | ||
| cacheData.put("source", "UPSTOX_WS"); | ||
|
|
||
| String json = objectMapper.writeValueAsString(cacheData); | ||
| String key = "market:latest-price:" + symbol; | ||
|
|
||
| // 6 hours TTL during market day fallback | ||
| redisTemplate.opsForValue().set(key, json, 6, TimeUnit.HOURS); | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("cacheLatestPrices", "Error caching latest prices to Redis", e); |
There was a problem hiding this comment.
Isolate per-symbol cache errors so one bad entry doesn’t drop the whole batch.
A single malformed quote/JSON currently aborts processing for all remaining symbols in both methods because error handling wraps the entire loop.
Proposed fix
public void cacheLatestPrices(Map<String, OHLCQuote> quotes) {
if (quotes == null || quotes.isEmpty()) {
return;
}
- try {
- for (Map.Entry<String, OHLCQuote> entry : quotes.entrySet()) {
+ for (Map.Entry<String, OHLCQuote> entry : quotes.entrySet()) {
+ try {
String rawSymbol = entry.getKey();
...
redisTemplate.opsForValue().set(key, json, 6, TimeUnit.HOURS);
+ } catch (Exception ex) {
+ log.warn("cacheLatestPrices", "Skipping symbol due to Redis write failure: " + entry.getKey(), ex);
}
- } catch (Exception e) {
- log.error("cacheLatestPrices", "Error caching latest prices to Redis", e);
}
}
public Map<String, OHLCQuote> getLatestPrices(Set<String> symbols) {
...
Map<String, OHLCQuote> result = new HashMap<>();
- try {
- for (String symbol : symbols) {
+ for (String symbol : symbols) {
+ try {
...
- result.put(symbol, quote);
+ result.put(cleanSymbol, quote);
+ } catch (Exception ex) {
+ log.warn("getLatestPrices", "Skipping symbol due to Redis read/parse failure: " + symbol, ex);
}
- } catch (Exception e) {
- log.error("getLatestPrices", "Error fetching latest prices from Redis", e);
}
return result;
}Also applies to: 760-795
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-service/src/main/java/com/am/marketdata/service/MarketDataCacheService.java`
around lines 705 - 747, The loop in MarketDataCacheService currently has a
single try/catch around the entire quotes.entrySet() iteration so one malformed
quote or JSON error aborts caching for all symbols; move error handling inside
the loop by wrapping each entry's processing (the body that normalizes
rawSymbol, builds cacheData, calls objectMapper.writeValueAsString and
redisTemplate.opsForValue().set) in its own try/catch, log the exception with
the rawSymbol/symbol and continue to the next entry; apply the same per-entry
try/catch refactor to the other similar method referenced (the block around
lines 760-795) so individual symbol failures don’t stop the batch.
| quote.setOhlc(ohlc); | ||
| quote.setPreviousClose(previousClose); | ||
|
|
||
| result.put(symbol, quote); |
There was a problem hiding this comment.
Use normalized key for returned map entries.
Line 790 stores result under the raw request symbol instead of cleanSymbol. That can cause downstream cache-join misses when casing/prefix formats differ.
Proposed fix
- result.put(symbol, quote);
+ result.put(cleanSymbol, quote);📝 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.
Suggested change
| result.put(symbol, quote); | |
| result.put(cleanSymbol, quote); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@am-market-data/market-data-service/src/main/java/com/am/marketdata/service/MarketDataCacheService.java`
at line 790, The map is being populated with the raw request key (`symbol`)
which can differ in casing/prefix from the normalized key (`cleanSymbol`) and
cause cache-join misses; update the insertion to use the normalized key by
replacing result.put(symbol, quote) with result.put(cleanSymbol, quote) (ensure
`cleanSymbol` is computed/available in the same scope and handle any null/empty
case consistently).
|
|
||
| # Auto-instrumentation is now configured at module-level to guarantee correct ASGI middleware execution order. | ||
|
|
||
| logger.info(f"Connecting to MongoDB: {settings.mongo_uri}") |
There was a problem hiding this comment.
Avoid logging the full MongoDB URI (credentials exposure).
Line 71 logs settings.mongo_uri, which commonly includes username/password. Log host/db metadata only.
Suggested fix
- logger.info(f"Connecting to MongoDB: {settings.mongo_uri}")
+ logger.info("Connecting to MongoDB", extra={"mongo_db": settings.mongo_db})📝 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.
Suggested change
| logger.info(f"Connecting to MongoDB: {settings.mongo_uri}") | |
| logger.info("Connecting to MongoDB", extra={"mongo_db": settings.mongo_db}) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/am_api/api.py` at line 71, The logger currently prints
settings.mongo_uri (exposing credentials); update the log to avoid printing the
full URI by parsing settings.mongo_uri and logging only safe metadata (host(s),
port(s), and database name) instead of credentials—modify the logging call
around logger.info(f"Connecting to MongoDB: {settings.mongo_uri}") to extract
and log host/db info (using a URI parser or pymongo.uri_parser.parse_uri) and
emit a message like "Connecting to MongoDB host(s): ..., db: ...".
Comment on lines
+28
to
+31
| provider = TracerProvider(resource=resource) | ||
| trace.set_tracer_provider(provider) | ||
| _TRACER_PROVIDER = provider | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In opentelemetry-python 1.20.0, what happens when trace.set_tracer_provider() is called after a tracer provider is already set, and what is the recommended pattern for library/app initialization?
💡 Result:
In OpenTelemetry Python 1.20.0, the global TracerProvider is intended to be set only once [1][2][3]. If trace.set_tracer_provider is called after a TracerProvider has already been initialized (either explicitly in code or automatically via environment variables/instrumentation), the subsequent call will be ignored, and the OpenTelemetry library will log a warning message stating: "Overriding of current TracerProvider is not allowed" [1][2][3]. This safeguard exists to ensure consistency and prevent the loss of span processors or telemetry data that might occur if the provider were replaced mid-execution [4][5]. Recommended Initialization Pattern: For applications, the recommended pattern is to initialize the TracerProvider as early as possible in the application's lifecycle, ideally before any other instrumentation or business logic that might trigger tracer creation is executed [6][7]. 1. Initialize the TracerProvider: Instantiate your TracerProvider and configure it with the necessary SpanProcessors, Exporters, and Resources [8][6]. 2. Set the Global Provider: Use trace.set_tracer_provider to register your configured provider as the global default [6]. 3. Consistent Use: Once the global provider is set, subsequent calls to trace.get_tracer will automatically use this configured provider, ensuring that all spans are captured and processed consistently [9][8]. Libraries should avoid setting the global TracerProvider themselves [2]. Instead, they should rely on the tracer provider provided by the application. If a library needs to create a tracer, it should obtain it via trace.get_tracer without specifying a provider, which will default to the global provider, or accept an optional TracerProvider as an argument if it needs to support custom configurations provided by the user [9][8].
Citations:
- 1: https://opentelemetry-python.readthedocs.io/en/latest/api/trace.html
- 2: https://opentelemetry-python.readthedocs.io/en/stable/%5Fmodules/opentelemetry/trace.html
- 3: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/__init__.py
- 4: Stop TracerProvider from being overridden open-telemetry/opentelemetry-python#959
- 5: Race condition in
set_tracer_provider()open-telemetry/opentelemetry-python#2181 - 6: https://opentelemetry.io/docs/languages/python/instrumentation/
- 7: https://opentelemetry.io/docs/languages/python/getting-started/
- 8: https://opentelemetry.io/docs/languages/python/cookbook/
- 9: https://opentelemetry-python.readthedocs.io/en/stable/api/trace.html?highlight=TracerProvider
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the file
sed -n '1,120p' am-parser/am_common/observability/tracing.py | nl -ba
# Also locate any other references to _TRACER_PROVIDER in the repo
rg -n "_TRACER_PROVIDER" -S .Repository: AM-Portfolio/am-market
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tracing.py (relevant top section) ==="
sed -n '1,140p' am-parser/am_common/observability/tracing.py
echo
echo "=== _TRACER_PROVIDER usages ==="
rg -n "_TRACER_PROVIDER" am-parser -S || true
echo
echo "=== configure_tracing definition (if present) ==="
rg -n "def +configure_tracing|configure_tracing" am-parser/am_common/observability/tracing.py -S || trueRepository: AM-Portfolio/am-market
Length of output: 3536
🌐 Web query:
In opentelemetry-python, what does trace.get_tracer_provider() return (real TracerProvider vs ProxyTracerProvider) when a tracer provider was already set?
💡 Result:
In opentelemetry-python, trace.get_tracer_provider returns the configured TracerProvider if one has already been set [1][2]. The ProxyTracerProvider is only returned if no global tracer provider has been set, either programmatically or via environment variables [1][2][3]. Specifically: - If a tracer provider has been set (typically via trace.set_tracer_provider), get_tracer_provider returns that instance [1][2]. - If no tracer provider has been set and the environment variable OTEL_PYTHON_TRACER_PROVIDER is not configured, it returns an instance of ProxyTracerProvider [1][2][3]. The ProxyTracerProvider acts as a placeholder that provides a no-op implementation until the real tracer provider is initialized [4][5]. Once the global provider is set, the proxies delegate calls to the underlying implementation [4][3]. Users often encounter the ProxyTracerProvider when attempting to access SDK-specific attributes (like resource) before the tracer provider has been properly initialized [6][7][5].
Citations:
- 1: https://opentelemetry-python.readthedocs.io/en/stable/%5Fmodules/opentelemetry/trace.html
- 2: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/__init__.py
- 3: https://opentelemetry-python.readthedocs.io/en/latest/_modules/opentelemetry/trace.html
- 4: Proxy tracer/provider to enable lazy setup of tracing pipeline open-telemetry/opentelemetry-python#1726
- 5: https://storiesfromtheherd.com/opentelemetry-python-manual-instrumentation-how-to-guide-b6c518a8427e
- 6: https://stackoverflow.com/questions/78976021/azure-monitor-open-telemetry-python-package-raising-exception-when-python-app-de
- 7: error with logging instrumentation - AttributeError: 'ProxyTracerProvider' object has no attribute 'resource' open-telemetry/opentelemetry-python-contrib#810
Reuse the already-registered global tracer provider before adding span processors (configure_tracing).
If another initializer sets the global TracerProvider first, trace.set_tracer_provider(...) is ignored, but this code still stores the newly created local provider in _TRACER_PROVIDER and attaches the OTLP BatchSpanProcessor to it—so those processors/exporter can silently receive no spans.
Proposed fix
- provider = TracerProvider(resource=resource)
- trace.set_tracer_provider(provider)
- _TRACER_PROVIDER = provider
+ existing_provider = trace.get_tracer_provider()
+ if isinstance(existing_provider, TracerProvider):
+ provider = existing_provider
+ else:
+ provider = TracerProvider(resource=resource)
+ trace.set_tracer_provider(provider)
+ _TRACER_PROVIDER = provider🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/am_common/observability/tracing.py` around lines 28 - 31, The code
currently always creates a new TracerProvider, sets it via
trace.set_tracer_provider(...), and stores it in _TRACER_PROVIDER which can
detach processors if another initializer already registered a global provider;
change configure_tracing to first call trace.get_tracer_provider() and use that
existing provider if it's already a real provider (not the default/no-op), only
create and set a new TracerProvider if none exists, then attach the
BatchSpanProcessor/exporter to the chosen provider (use the symbols provider,
_TRACER_PROVIDER, trace.get_tracer_provider, trace.set_tracer_provider, and
BatchSpanProcessor so the processor is always registered to the active global
provider).
| df.dropna(how='all', inplace=True) | ||
|
|
||
| print(f"INFO: Sheet dimensions: {df.shape[0]} rows x {df.shape[1]} columns") | ||
| logger.info(f"Sheet dimensions", extra={"rows": df.shape[0], "columns": df.shape[1]}) |
There was a problem hiding this comment.
Remove f-string prefixes where no interpolation is used.
Lines 77, 234, and 277 are F541 lint errors.
Also applies to: 234-234, 277-277
🧰 Tools
🪛 Ruff (0.15.14)
[error] 77-77: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/am_llm/together_service.py` at line 77, The lint errors come from
using f-strings with no interpolation in logger.info calls; locate the
logger.info calls in together_service.py (the one that logs "Sheet dimensions"
and the other two logger.info occurrences around the later df/shape logs) and
remove the leading f from those string literals (e.g., change f"Sheet
dimensions" to "Sheet dimensions") so only strings that actually interpolate use
the f-prefix; keep any f-strings that do include placeholders intact.
Comment on lines
+191
to
+194
| # Apply custom API Key if provided | ||
| if api_key: | ||
| os.environ["TOGETHER_API_KEY"] = api_key | ||
|
|
There was a problem hiding this comment.
Do not store per-request API keys in process-global os.environ.
Line 193 mutates global state with tenant/request secret data, which can leak keys across concurrent requests and cause racey authentication behavior.
Suggested remediation direction
- if api_key:
- os.environ["TOGETHER_API_KEY"] = api_key
-
- result = await self._parse_sheet_file(sheet_file, method)
+ result = await self._parse_sheet_file(sheet_file, method, api_key=api_key)Then thread api_key through _parse_sheet_file(...) and _sync_parse_file(...), and instantiate TogetherLLMService(api_key=api_key) locally instead of mutating environment state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/am_services/file_processing_service.py` around lines 191 - 194, The
code currently sets per-request API keys in process-global os.environ which
risks leaking secrets and race conditions; remove the os.environ mutation and
instead thread the api_key parameter into the parsing call chain
(_parse_sheet_file and _sync_parse_file) and construct a local
TogetherLLMService using TogetherLLMService(api_key=api_key) where needed (e.g.,
inside the file processing handler and any helper that uses the LLM) so each
request uses its own service instance without mutating global environment state.
| }).to_list(None) | ||
|
|
||
| print(f"🔍 Found {len(stuck_jobs)} potentially stuck jobs") | ||
| logger.info(f"Found potentially stuck jobs", extra={"stuck_count": len(stuck_jobs)}) |
There was a problem hiding this comment.
Fix invalid f-strings on structured logs.
Lines 103 and 107 use f-strings without placeholders (F541).
Also applies to: 107-107
🧰 Tools
🪛 Ruff (0.15.14)
[error] 103-103: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/am_services/job_queue_service.py` at line 103, The logger.info
calls use unnecessary f-strings (e.g., the call that passes
extra={"stuck_count": len(stuck_jobs)}) which triggers F541; remove the f-prefix
so the message is a plain string (or use a proper formatted placeholder) and
keep the structured extra payload intact—update both logger.info invocations in
job_queue_service.py that mention stuck jobs (the one using
stuck_jobs/stuck_count and the similar persistent-stuck log) to use non-f string
literals.
Comment on lines
+113
to
+117
| root_logger = logging.getLogger() | ||
| for h in root_logger.handlers[:]: | ||
| root_logger.removeHandler(h) | ||
| root_logger.addHandler(handler) | ||
|
|
There was a problem hiding this comment.
Restore logger state after each test to avoid cross-test pollution.
On Line 113 and Line 147 (and other handler overrides in this file), tests mutate global/root and named logger handlers but never restore them. This can make unrelated tests flaky depending on execution order.
Proposed diff (fixture-based isolation)
import pytest
@@
from io import StringIO
@@
from am_common.logging.core import AMLogger
+
+
+@pytest.fixture(autouse=True)
+def _restore_logging_state():
+ root = logging.getLogger()
+ root_handlers = root.handlers[:]
+ root_level = root.level
+ try:
+ yield
+ finally:
+ root.handlers = root_handlers
+ root.setLevel(root_level)
+ clear_context()Also applies to: 147-151, 68-71, 190-193, 286-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@am-parser/tests/test_observability.py` around lines 113 - 117, Tests in
test_observability.py mutate global and named logger state (e.g., root_logger =
logging.getLogger(), removing handlers and adding handler) without restoring it,
causing cross-test pollution; fix by wrapping these manipulations in a pytest
fixture or try/finally that saves the original logger state (handlers, level,
propagate, and any named logger handlers) before changes and restores them after
the test, updating occurrences around the root_logger variable and any places
using handler, logging.getLogger(), or specific named logger instances so each
test resets to the original handlers/levels on teardown.
ssd2658
had a problem deploying
to
preprod-am-parser
GitHub Actions
Failure
ssd2658
had a problem deploying
to
preprod-am-parser
GitHub Actions
Failure
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements