Migrate from Firebase Functions logging to standard Python logging - #97
Conversation
Summary of ChangesHello @SuperMuel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the backend's logging infrastructure. The primary goal is to standardize logging practices by transitioning from Firebase Functions' native logger to Python's robust Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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. WalkthroughReplaces firebase_functions logger usage with Python stdlib logging across many modules, adds FirebaseFunctionsHandler and configure_firebase_functions_logging in a new logging_config, and updates main.py to configure and use the new logging pipeline. No functional control-flow changes beyond logging setup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Module as App Module (e.g., service, sync, main)
participant StdLog as Python logging (root / module logger)
participant FFH as FirebaseFunctionsHandler
participant FF as Firebase Functions Logger
Module->>StdLog: logger.info("msg", extra={...})
Note right of Module: module-level logging.getLogger(__name__)
StdLog->>FFH: Emit LogRecord (level,msg,extras)
Note right of FFH: maps level → severity, filters extras
FFH->>FF: firebase_functions.logger.log(severity, message, **extras)
FF-->>FFH: ack
FFH-->>StdLog: handled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used📓 Path-based instructions (1)backend/**/*.py📄 CodeRabbit inference engine (backend/AGENTS.md)
Files:
🧬 Code graph analysis (4)backend/backend/services/ics_service.py (3)
backend/backend/services/sync_profile_service.py (3)
backend/backend/infrastructure/event_bus.py (2)
backend/backend/repositories/sync_profile_repository.py (1)
⏰ 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). (1)
🔇 Additional comments (11)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 74.31% 73.45% -0.87%
==========================================
Files 42 42
Lines 1908 1944 +36
==========================================
+ Hits 1418 1428 +10
- Misses 490 516 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the backend from Firebase Functions logging to the standard Python logging module, which is a great improvement for consistency and performance. The new FirebaseFunctionsHandler provides a clean bridge to Firebase's structured logging. The transition to parameterized logging has been applied widely, though I've noted a couple of spots where f-strings remain. I also found a potential issue in the logging configuration related to clearing handlers. Overall, these are excellent changes that standardize the logging approach.
| if root_logger.handlers: | ||
| for handler in root_logger.handlers: | ||
| root_logger.removeHandler(handler) |
There was a problem hiding this comment.
Modifying root_logger.handlers while iterating over it can lead to unexpected behavior. A more direct and safer way to remove all handlers is to use root_logger.handlers.clear().
root_logger.handlers.clear()| logger.warning( | ||
| f"Calendar not found (404) for ID: {calendar_id}", | ||
| user_id=user_id, | ||
| provider_account_id=provider_account_id, | ||
| extra={ | ||
| "user_id": user_id, | ||
| "provider_account_id": provider_account_id, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
To fully embrace parameterized logging for performance and consistency, it's best to avoid f-strings in log messages. The variable part of the message should be passed as a separate argument.
| logger.warning( | |
| f"Calendar not found (404) for ID: {calendar_id}", | |
| user_id=user_id, | |
| provider_account_id=provider_account_id, | |
| extra={ | |
| "user_id": user_id, | |
| "provider_account_id": provider_account_id, | |
| }, | |
| ) | |
| logger.warning( | |
| "Calendar not found (404) for ID: %s", | |
| calendar_id, | |
| extra={ | |
| "user_id": user_id, | |
| "provider_account_id": provider_account_id, | |
| }, | |
| ) |
| logger.info( | ||
| f"Validating existing target calendar: {calendar_id}", | ||
| user_id=user_id, | ||
| extra={"user_id": user_id}, | ||
| ) |
There was a problem hiding this comment.
To maintain consistency with the new parameterized logging style and gain its performance benefits, this f-string should be converted to use a format string with arguments.
logger.info(
"Validating existing target calendar: %s",
calendar_id,
extra={"user_id": user_id},
)![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
backend/backend/synchronizer/google_calendar_manager.py (3)
109-123: Batch requests lack per-item error handling; add callback to detect partial failuresCurrent batch execution drops per-request errors. Use a callback to log/report failures and consider retrying failed items to avoid silent data loss.
Suggested change:
- batch = self._service.new_batch_http_request() + def _insert_cb(request_id, response, exception): + if exception: + logger.error( + "Failed to insert event in batch (request_id=%s): %s", + request_id, + exception, + extra={"calendar_id": self._calendar_id, "sync_profile_id": sync_profile_id}, + ) + + batch = self._service.new_batch_http_request(callback=_insert_cb) for event in sublist: google_event = self._event_to_google_event( event, extended_properties=self._create_extended_properties( sync_profile_id ), ) batch.add( self._service.events().insert( calendarId=self._calendar_id, body=google_event, ) ) batch.execute()You can mirror this pattern in delete_events.
124-124: Progress computation uses hard-coded 50; should respect batch_sizeThe log uses i * 50 which becomes incorrect when batch_size != 50.
Apply:
- logger.info(f"Inserted {i * 50 + len(sublist)}/{len(events)} events.") + inserted = i * batch_size + len(sublist) + logger.info( + "Inserted %d/%d events", + inserted, + len(events), + extra={"calendar_id": self._calendar_id, "sync_profile_id": sync_profile_id}, + )
207-219: HttpError status check is incorrect; usee.resp.statusand preferlogger.exceptiongoogleapiclient.errors.HttpError exposes HTTP code via resp.status, not status_code. Current code may mis-handle 404. Also, use logger.exception for unexpected errors to capture stack traces.
- except HttpError as e: - if e.status_code == 404: - logger.info(f"Calendar {self._calendar_id} not found") + except HttpError as e: + if hasattr(e, "resp") and getattr(e.resp, "status", None) == 404: + logger.info( + "Calendar %s not found", + self._calendar_id, + extra={"calendar_id": self._calendar_id}, + ) return False raise - except Exception as e: - logger.error( - f"Unexpected error checking calendar: {e.__class__.__name__}: {e}" - ) + except Exception: + logger.exception( + "Unexpected error checking calendar", + extra={"calendar_id": self._calendar_id}, + ) raisebackend/backend/synchronizer/ics_source.py (1)
121-124: Do not log full response headers; potential PII/secret leakageDumping response.headers can leak cookies or other sensitive data. Remove it and log only safe fields.
- logger.info( - f"Content-Length is too large ({content_length / 1_048_576:.2f}MB > {max_content_size_b / 1_048_576:.2f}MB) ({response.headers=})" - ) + logger.info( + "Content-Length too large (%.2fMB > %.2fMB)", + content_length / 1_048_576, + max_content_size_b / 1_048_576, + extra={"url": str(self.url), "content_length": content_length}, + )backend/backend/services/google_calendar_service.py (1)
135-145: Non-404 HttpError is swallowed, causing silent None returns.The
except HttpErrorblock handles 404 but does not re-raise for other statuses, effectively swallowing errors like 401/403/5xx.Apply this diff to re-raise non-404s and keep structured context:
- except HttpError as e: - # Check if the error is specifically a 404 Not Found - if e.resp.status == 404: - logger.warning( - f"Calendar not found (404) for ID: {calendar_id}", - extra={ - "user_id": user_id, - "provider_account_id": provider_account_id, - }, - ) - return None + except HttpError as e: + # Handle 404 (not found) explicitly; re-raise others + if getattr(e, "resp", None) and getattr(e.resp, "status", None) == 404: + logger.warning( + "Calendar not found (404) for ID: %s", + calendar_id, + extra={ + "user_id": user_id, + "provider_account_id": provider_account_id, + }, + ) + return None + logger.error( + "Calendar API error (non-404) for ID: %s", + calendar_id, + extra={ + "user_id": user_id, + "provider_account_id": provider_account_id, + }, + exc_info=True, + ) + raise BaseTargetCalendarError( + message="Failed to get calendar by id", original_exception=e + )backend/backend/services/dev_notification_service.py (1)
176-182: Avoid logging full Telegram response bodies; parameterize and include context.
response.textmay contain sensitive content (user IDs/emails embedded in the message). Log status/summary instead; uselogger.exceptionfor exceptions.Apply this diff:
- if not response.ok: - logger.warning( - f"Failed to send Telegram notification: {response.text}" - ) + if not response.ok: + logger.warning( + "Failed to send Telegram notification: status=%s reason=%s", + response.status_code, + getattr(response, "reason", None), + extra={"chat_id": self.chat_id}, + ) @@ - except Exception as e: - logger.warning(f"Error sending Telegram notification: {str(e)}") + except Exception: + logger.exception( + "Error sending Telegram notification", + extra={"chat_id": self.chat_id}, + )As per coding guidelines
backend/backend/logging_config.py (1)
59-62: Avoid mutatinghandlerswhile iterating.Iteration while removing can skip handlers. Use a copy or clear().
Apply this diff:
- if root_logger.handlers: - for handler in root_logger.handlers: - root_logger.removeHandler(handler) + if root_logger.handlers: + for handler in list(root_logger.handlers): + root_logger.removeHandler(handler)Apply the same change in
configure_firebase_functions_logging.Also applies to: 97-100
backend/main.py (2)
358-386: CRITICAL: Remove sensitive credential from logs.Line 362 logs
request.model_dump_json(), which includes theauthCodefield. OAuth authorization codes are sensitive credentials that must never be logged, as they grant access to user accounts.Apply this diff to log only non-sensitive fields:
logger.info( "Authorizing backend.", extra={ "user_id": user_id, - "request_payload": request.model_dump_json(), "redirect_uri": str(request.redirectUri), "provider_account_id": request.providerAccountId, }, )Note: The
authCodeis already excluded from the error log (lines 375-383), which is correct.
446-467: Avoid logging full request payload containing URL credentials.request.model_dump_json()includesscheduleSource.url, which can embed user:pass—log only non-sensitive fields (e.g.titleandtargetCalendar.id) in both the info and error logs.
🧹 Nitpick comments (16)
backend/backend/synchronizer/google_calendar_manager.py (2)
14-14: Adopt parameterized logging and add contextual extrasNow that the stdlib logger is in place, please switch f-strings below to parameterized logging and attach useful context (calendar_id, sync_profile_id) via extra for consistency and performance.
Example:
-logger.info(f"Creating {len(events)} events.") +logger.info( + "Creating %d events", + len(events), + extra={"calendar_id": self._calendar_id, "sync_profile_id": sync_profile_id}, +)
186-191: Avoid shadowing built-inidRename loop variable to prevent confusion and improve readability.
- for id in sublist: + for event_id in sublist: batch.add( self._service.events().delete( - calendarId=self._calendar_id, - eventId=id, + calendarId=self._calendar_id, + eventId=event_id, ) )backend/backend/services/authorization_service.py (4)
72-72: Switch to parameterized logging with contextual extrasUse printf-style formatting and attach user/account context via extra for consistency and performance.
- logger.info(f"Authorizing user {user_id} with auth code") + logger.info( + "Authorizing user %s with auth code", + user_id, + extra={"user_id": user_id}, + )Apply similarly to other info/debug logs in this class (e.g., fetching service, refreshing creds, authorization valid).
95-99: Uselogger.exceptionin exception handlers; avoid interpolatingeCaptures traceback automatically and avoids embedding potentially sensitive error strings.
- logger.error(f"Error exchanging authorization code: {e}") + logger.exception( + "Error exchanging authorization code", + extra={"user_id": user_id}, + ) ... - logger.error(f"Error verifying ID token: {e}") + logger.exception( + "Error verifying ID token", + extra={"user_id": user_id}, + ) ... - logger.error(f"Error refreshing Google credentials: {e}") + logger.exception( + "Error refreshing Google credentials", + extra={"user_id": user_id, "provider_account_id": provider_account_id}, + ) ... - logger.error(f"Failed to test authorization: {e}") + logger.exception( + "Failed to test authorization", + extra={"user_id": user_id, "provider_account_id": provider_account_id}, + )Also applies to: 117-121, 226-229, 262-266
201-203: Parameterize and add context to service fetch log- logger.info(f"Fetching calendar service for {user_id}/{provider_account_id}") + logger.info( + "Fetching calendar service for user %s / account %s", + user_id, + provider_account_id, + extra={"user_id": user_id, "provider_account_id": provider_account_id}, + )
221-229: Persist refreshed credentials (access token/expiry) after refreshCurrently TODO; without saving, subsequent calls may use stale tokens.
if not credentials.valid and credentials.refresh_token: logger.info("Refreshing Google credentials.") try: credentials.refresh(Request()) - # TODO : Save credentials after refresh + # Persist updated tokens + self._auth_repo.set_authorization( + BackendAuthorization( + userId=user_id, + provider="google", + providerAccountId=provider_account_id, + providerAccountEmail=self.get_provider_account_email( + user_id, provider_account_id + ), + accessToken=credentials.token, + refreshToken=credentials.refresh_token, + expirationDate=credentials.expiry, + ) + )I can adapt this to your repository’s exact repository API if you prefer.
backend/backend/synchronizer/ics_cache.py (1)
63-63: Parameterize log and attach useful contextUse printf-style formatting and include sync_profile_id and filename for traceability.
- logger.info(f"Stored ics string in firebase storage: {filename}") + logger.info( + "Stored ICS string in Firebase Storage: %s", + filename, + extra={"sync_profile_id": sync_profile_id, "filename": filename}, + )backend/backend/synchronizer/ics_source.py (2)
12-12: Convert f-string logs to parameterized logging with extrasSeveral logs in this module still use f-strings (Lines 103, 113, 121-123, 139, 143). Please switch to parameterized logging and attach relevant context (url, sizes) via extra.
Example changes:
- logger.info(f"Fetching ICS file from {self.url}") + logger.info("Fetching ICS file from %s", self.url, extra={"url": str(self.url)}) ... - logger.info(f"Content-Type is not text : {content_type}") + logger.info("Content-Type is not text: %s", content_type, extra={"url": str(self.url)}) ... - logger.info( - f"Content-Length is too large ({content_length / 1_048_576:.2f}MB > {max_content_size_b / 1_048_576:.2f}MB) ({response.headers=})" - ) + logger.info( + "Content-Length too large (%.2fMB > %.2fMB)", + content_length / 1_048_576, + max_content_size_b / 1_048_576, + extra={"url": str(self.url), "content_length": content_length}, + ) ... - logger.info(f"ICS string size: {len(s) / 1024} KB") + logger.info("ICS string size: %.2f KB", len(s) / 1024, extra={"url": str(self.url)})
142-144: Preferlogger.exceptionto capture stack trace on request failures- except requests.RequestException as e: - logger.error(f"Could not fetch ICS file : {e}") - raise IcsSourceError(f"Could not fetch ICS file. ", original_exception=e) + except requests.RequestException as e: + logger.exception( + "Could not fetch ICS file", + extra={"url": str(self.url)}, + ) + raise IcsSourceError("Could not fetch ICS file.", original_exception=e)backend/backend/services/ai_ruleset_service.py (1)
115-129: Uselogger.exceptionand parameterized logs for errors and results
- Replace f-string error logs with logger.exception to include tracebacks.
- Parameterize the "Generated ruleset" log and add context.
- except Exception as e: - logger.error(f"Failed to generate ruleset: {e}") + except Exception as e: + logger.exception( + "Failed to generate ruleset", + extra={"sync_profile_id": sync_profile.id, "user_id": sync_profile.user_id}, + ) sync_profile.update_ruleset( error=f"Failed to generate ruleset: {type(e).__name__}: {str(e)}" ) ... - logger.info(f"Generated ruleset: {str(output.ruleset)[:100]}...") + logger.info( + "Generated ruleset: %s...", + str(output.ruleset)[:100], + extra={"sync_profile_id": sync_profile.id, "user_id": sync_profile.user_id}, + )Also applies to: 132-133
backend/backend/services/google_calendar_service.py (2)
46-53: Good use of structured extras here. Standardize this across the file.Pattern looks right. For consistency with the PR goal, convert remaining f-string logs to parameterized logs and include extra where helpful.
As per coding guidelines
59-64: Prefer parameterized logs and include stack traces for errors.Replace f-strings in exception paths with
logger.exceptionorexc_info=Truefor better diagnostics and performance.Apply this diff:
- logger.error(f"Failed to list calendars: {e}") + logger.exception( + "Failed to list calendars", + extra={ + "user_id": user_id, + "provider_account_id": provider_account_id, + "max_calendars": max_calendars, + }, + ) @@ - logger.error(f"Failed to create calendar: {e}") + logger.exception( + "Failed to create calendar", + extra={ + "user_id": user_id, + "provider_account_id": provider_account_id, + "summary": summary, + }, + ) @@ - logger.error(f"Failed to get calendar by id: {e}") + logger.exception( + "Failed to get calendar by id", + extra={ + "user_id": user_id, + "provider_account_id": provider_account_id, + "calendar_id": calendar_id, + }, + )As per coding guidelines
Also applies to: 108-113, 145-147
backend/backend/ai/ruleset_builder.py (2)
105-115: Use parameterized logging for performance and consistency.Convert f-strings to parameterized logs.
Apply this diff:
- logger.info(f"Compressing {len(events)} events") + logger.info("Compressing %d events", len(events)) @@ - logger.info(f"Compression ratio: {compression_ratio:.2f}") + logger.info("Compression ratio: %.2f", compression_ratio)As per coding guidelines
25-31: Nit: avoid shadowing the built-ininput.Consider renaming parameter to
input_textto improve clarity.backend/backend/services/sync_profile_service.py (1)
513-516: Make logging fully parameterized and uselogger.exceptionfor errors.Most logs follow the new style with
extra; a few still use f-strings and miss stack traces.Apply these diffs (apply similarly elsewhere):
- logger.info( - f"Validating existing target calendar: {calendar_id}", - extra={"user_id": user_id}, - ) + logger.info( + "Validating existing target calendar: %s", + calendar_id, + extra={"user_id": user_id}, + ) @@ - logger.error( - "Failed to perform initial sync after profile creation.", - extra={ - "user_id": user_id, - "sync_profile_id": sync_profile_id, - "error_type": type(e).__name__, - "error_message": str(e), - }, - ) + logger.exception( + "Failed to perform initial sync after profile creation.", + extra={ + "user_id": user_id, + "sync_profile_id": sync_profile_id, + }, + )Optional: consider logging a WARNING (not INFO) when the daily sync limit is reached to make it more visible in metrics.
As per coding guidelinesAlso applies to: 600-608
backend/backend/logging_config.py (1)
81-85: Also setuvicorn.accesslogger level.Uvicorn access logs use a separate logger.
Apply this diff:
# Set Uvicorn access logs to the specified level - logging.getLogger("uvicorn").setLevel(level) + logging.getLogger("uvicorn").setLevel(level) + logging.getLogger("uvicorn.access").setLevel(level)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/backend/ai/ruleset_builder.py(2 hunks)backend/backend/logging_config.py(2 hunks)backend/backend/services/ai_ruleset_service.py(3 hunks)backend/backend/services/authorization_service.py(2 hunks)backend/backend/services/dev_notification_service.py(4 hunks)backend/backend/services/google_calendar_service.py(3 hunks)backend/backend/services/sync_profile_service.py(9 hunks)backend/backend/synchronizer/google_calendar_manager.py(1 hunks)backend/backend/synchronizer/ics_cache.py(1 hunks)backend/backend/synchronizer/ics_source.py(1 hunks)backend/main.py(18 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/**/*.py: Prefer Pydantic models over raw dictionaries for input validation
Favor functional, declarative style with pure functions and immutable data structures where practical
Handle errors and edge cases at the beginning of functions
Use early returns for error conditions to avoid deeply nested if statements
Place the happy path last in the function for improved readability
Avoid unnecessary else statements; use the if-return pattern instead
Use guard clauses to handle preconditions and invalid states early
Implement proper error logging and user-friendly error messages
Use assert to catch developer errors
Use docstrings for all public functions, methods, and classes
Write docstrings for public APIs that explain usage and purpose, not implementation
Focus docstrings on why to use and how to use correctly
Skip docstrings for trivial functions where name and type hints are self-explanatory
Prefer self-documenting code over comments; add comments for complex business logic or why-decisions
Use type hints throughout the codebase
Prefer built-in typing syntax like list[str | None] over List[Optional[str]]
Write testable code by minimizing patching and favoring dependency injection
Files:
backend/backend/synchronizer/ics_cache.pybackend/backend/synchronizer/ics_source.pybackend/backend/ai/ruleset_builder.pybackend/backend/services/dev_notification_service.pybackend/backend/services/sync_profile_service.pybackend/backend/services/ai_ruleset_service.pybackend/main.pybackend/backend/services/authorization_service.pybackend/backend/synchronizer/google_calendar_manager.pybackend/backend/services/google_calendar_service.pybackend/backend/logging_config.py
🧬 Code graph analysis (5)
backend/backend/synchronizer/ics_cache.py (1)
backend/backend/synchronizer/ics_source.py (2)
IcsSource(15-31)UrlIcsSource(34-144)
backend/backend/synchronizer/ics_source.py (1)
backend/backend/services/exceptions/ics.py (1)
IcsSourceError(10-13)
backend/backend/services/sync_profile_service.py (2)
backend/backend/synchronizer/ics_source.py (1)
UrlIcsSource(34-144)backend/tests/services/test_sync_profile_service.py (1)
uuid_factory(939-940)
backend/main.py (3)
backend/backend/logging_config.py (1)
configure_firebase_functions_logging(87-107)backend/backend/services/authorization_service.py (1)
test_authorization(234-266)backend/backend/services/sync_profile_service.py (1)
create_sync_profile(422-614)
backend/backend/services/google_calendar_service.py (2)
backend/backend/services/authorization_service.py (1)
AuthorizationService(29-278)backend/backend/services/exceptions/target_calendar.py (1)
BaseTargetCalendarError(4-7)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
backend/backend/services/ai_ruleset_service.py (1)
67-74: Good migration to parameterized logging with structured contextThis aligns with the PR’s goals and the repo’s logging direction. LGTM.
backend/backend/services/dev_notification_service.py (1)
142-157: Nice: parameterized logs with contextual extras.Good move to
%sformatting andextra={"user_id": ...}. Keep this pattern.backend/main.py (1)
1-60: LGTM! Logging configuration is set up correctly.The migration to standard Python logging is well-structured:
- Module-level logger follows Python conventions
- Logging configuration is properly initialized before use
- Parameterized logging for settings improves performance
| extras = { | ||
| key: value | ||
| for key, value in record.__dict__.items() | ||
| if key not in _RESERVED_LOG_RECORD_ATTRS and not key.startswith("_") | ||
| } | ||
|
|
||
| entry: dict[str, object] = {"severity": severity} | ||
| if message: | ||
| entry["message"] = message | ||
| if extras: | ||
| entry.update(extras) | ||
|
|
||
| firebase_logger.write(cast(firebase_logger.LogEntry, entry)) | ||
|
|
There was a problem hiding this comment.
Coerce extra values to JSON-friendly types before writing to Firebase.
Non-serializable extras can break firebase_logger.write. Safely coerce complex types.
Apply this diff:
- extras = {
- key: value
- for key, value in record.__dict__.items()
- if key not in _RESERVED_LOG_RECORD_ATTRS and not key.startswith("_")
- }
+ extras_raw = {
+ key: value
+ for key, value in record.__dict__.items()
+ if key not in _RESERVED_LOG_RECORD_ATTRS and not key.startswith("_")
+ }
+ # Coerce extras to JSON-friendly shapes
+ def _coerce(v):
+ try:
+ import json # local import to avoid global dependency if unused
+ json.dumps(v)
+ return v
+ except Exception:
+ return repr(v)
+ extras = {k: _coerce(v) for k, v in extras_raw.items()}📝 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.
| extras = { | |
| key: value | |
| for key, value in record.__dict__.items() | |
| if key not in _RESERVED_LOG_RECORD_ATTRS and not key.startswith("_") | |
| } | |
| entry: dict[str, object] = {"severity": severity} | |
| if message: | |
| entry["message"] = message | |
| if extras: | |
| entry.update(extras) | |
| firebase_logger.write(cast(firebase_logger.LogEntry, entry)) | |
| extras_raw = { | |
| key: value | |
| for key, value in record.__dict__.items() | |
| if key not in _RESERVED_LOG_RECORD_ATTRS and not key.startswith("_") | |
| } | |
| # Coerce extras to JSON-friendly shapes | |
| def _coerce(v): | |
| try: | |
| import json # local import to avoid global dependency if unused | |
| json.dumps(v) | |
| return v | |
| except Exception: | |
| return repr(v) | |
| extras = {k: _coerce(v) for k, v in extras_raw.items()} | |
| entry: dict[str, object] = {"severity": severity} | |
| if message: | |
| entry["message"] = message | |
| if extras: | |
| entry.update(extras) | |
| firebase_logger.write(cast(firebase_logger.LogEntry, entry)) |
| def configure_firebase_functions_logging(log_level: str = "INFO") -> None: | ||
| """ | ||
| Configure logging so stdlib log calls are routed through the Firebase logger. | ||
|
|
||
| Args: | ||
| log_level: The logging level to use (DEBUG, INFO, WARNING, ERROR, CRITICAL) | ||
| """ | ||
| root_logger = logging.getLogger() | ||
|
|
||
| # Clear existing handlers to avoid duplicate logging | ||
| if root_logger.handlers: | ||
| for handler in root_logger.handlers: | ||
| root_logger.removeHandler(handler) | ||
|
|
||
| level = getattr(logging, log_level.upper()) | ||
| root_logger.setLevel(level) | ||
|
|
||
| handler = FirebaseFunctionsHandler() | ||
| handler.setLevel(level) | ||
| handler.setFormatter(logging.Formatter("%(message)s")) | ||
| root_logger.addHandler(handler) | ||
|
|
||
| __all__ = [ | ||
| "configure_logging", | ||
| "configure_firebase_functions_logging", | ||
| "FirebaseFunctionsHandler", | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Repo-wide verification: ensure no leftover firebase logger usage and f-string logs.
Scan for remaining firebase_functions.logger usage and f-strings in logging calls.
🏁 Script executed:
#!/bin/bash
# Check for legacy firebase logger usage
rg -nP 'firebase_functions\.logger' -S
# Find f-strings passed to logger calls (common patterns)
rg -nP --type=py -C1 '\blogger\.(debug|info|warning|error|exception|critical)\(\s*f"' | sed -E 's/^/FSTR: /'
# Find logger.error without exc_info in except blocks (heuristic)
rg -nP --type=py -C1 'except\s+.+\s+as\s+e:\s*\n\s*logger\.(error|warning)\('Length of output: 17572
No legacy Firebase logger usage; refactor f-string logging
No references to firebase_functions.logger remain. Many logger.*(f"...") calls (e.g., backend/backend/api.py:38, backend/backend/synchronizer/ics_cache.py:63) should be changed to use lazy formatting with %s placeholders and argument lists.
🤖 Prompt for AI Agents
In backend/backend/logging_config.py around lines 87 to 113, ensure there are no
references to firebase_functions.logger and that the logging configuration
remains compatible with standard library logging; then replace all f-string log
calls across the codebase (examples: backend/backend/api.py:38,
backend/backend/synchronizer/ics_cache.py:63) with lazy %-style formatting and
argument lists (e.g. change logger.info(f"User {user} created") to
logger.info("User %s created", user)), ensuring variables are passed as separate
arguments so formatting is deferred and consistent with the configured handler.
| if root_logger.handlers: | ||
| for handler in root_logger.handlers: | ||
| root_logger.removeHandler(handler) | ||
|
|
There was a problem hiding this comment.
Bug: Logging Configuration Iteration Bug
The configure_firebase_functions_logging function modifies root_logger.handlers while iterating to clear them, which can cause handlers to be skipped. Additionally, it doesn't close handlers, potentially leading to resource leaks. This issue was already addressed in the configure_logging function.
2 participants
Migrates the backend from Firebase Functions logging to standard Python logging.
Key Changes:
firebase_functions.loggerwith standardloggingmoduleFirebaseFunctionsHandlerto bridge Python logging to Firebase Functionsmain.pyto use new logging configurationBenefits:
Summary by CodeRabbit
Refactor
Chores