Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates backend behavior around authentication cookie security configuration and adds additional logging for messaging-service ingestion to improve observability.
Changes:
- Type-annotates
AtlasAppconfiguration and wiresuse_sslfrom app config intoUserRouterto control the cookiesecureflag. - Adds INFO-level logs when handling messaging-service messages (Signal / Teams / SciLog) in the message service ingestor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/bec_atlas/main.py | Passes use_ssl from app config into UserRouter; adds config type annotation. |
| backend/bec_atlas/ingestor/message_service_ingestor.py | Adds INFO logs around message processing per service type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.info( | ||
| f"Processed signal message with scope {msg.scope} for deployment {deployment.deployment_id}." |
There was a problem hiding this comment.
This INFO log runs for every ingested message and also reports "Processed" unconditionally after calling self.signal_manager.process(...), even though that method can early-return without sending anything (e.g., missing scope / missing group). Consider (1) moving the success log into the manager after the actual send (or returning a status), and (2) using DEBUG or a config-gated/sampled INFO log to avoid high-volume log noise.
| logger.info( | |
| f"Processed signal message with scope {msg.scope} for deployment {deployment.deployment_id}." | |
| logger.debug( | |
| f"Handled signal message with scope {msg.scope} for deployment {deployment.deployment_id}." |
| logger.info( | ||
| f"Received message for Microsoft Teams with scope {msg.scope} for deployment {deployment.deployment_id}." | ||
| ) | ||
| pass # not implemented yet |
There was a problem hiding this comment.
The Teams branch currently logs that a message was received but then immediately passes (message is effectively dropped). This log message/level is misleading for operators; consider logging a WARNING (or ERROR) that the service is not implemented and the message is being ignored (and/or explicitly acknowledge that behavior).
| logger.info( | |
| f"Received message for Microsoft Teams with scope {msg.scope} for deployment {deployment.deployment_id}." | |
| ) | |
| pass # not implemented yet | |
| logger.warning( | |
| f"Ignoring message for Microsoft Teams with scope {msg.scope} for deployment {deployment.deployment_id}: " | |
| "Teams messaging service is not implemented yet." | |
| ) |
| self.scilog_manager.process(msg, deployment) | ||
| logger.info( | ||
| f"Processed SciLog message with scope {msg.scope} for deployment {deployment.deployment_id}." | ||
| ) |
There was a problem hiding this comment.
This log line reports "Processed" unconditionally after calling self.scilog_manager.process(...), but that method can early-return without ingesting anything (e.g., when scope is not allowed). Consider only logging success when an actual ingest occurred (e.g., have process return a boolean/result, or move the success log into the manager after the ingest).
| self.scilog_manager.process(msg, deployment) | |
| logger.info( | |
| f"Processed SciLog message with scope {msg.scope} for deployment {deployment.deployment_id}." | |
| ) | |
| ingested = self.scilog_manager.process(msg, deployment) | |
| if ingested: | |
| logger.info( | |
| f"Processed SciLog message with scope {msg.scope} for deployment {deployment.deployment_id}." | |
| ) | |
| else: | |
| logger.info( | |
| f"SciLog message with scope {msg.scope} for deployment {deployment.deployment_id} was not ingested." | |
| ) |
No description provided.