feat: abstract logging tool integration #787
Conversation
Please make sure all the checkboxes are checked:
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update refactors the observability and monitoring infrastructure throughout the codebase. The legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Module as Any Module (e.g., Adapter)
participant GetObserve as get_observe()
participant Config as BaseConfig
participant ObserverEnum as Observer
participant Decorator as langfuse.decorators.observe
Module->>GetObserve: Call get_observe()
GetObserve->>Config: Retrieve monitoring_tool from config
GetObserve->>ObserverEnum: Compare tool to Observer.LANGFUSE
alt If LANGFUSE
GetObserve->>Decorator: Import observe decorator
GetObserve-->>Module: Return observe
else Not supported
GetObserve-->>Module: Return None
end
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cognee/modules/observability/get_observe.py (1)
1-12: 🛠️ Refactor suggestionAdd handling for non-LANGFUSE observers and include docstring
This new module effectively centralizes the observability decorator acquisition, but has two issues:
It doesn't explicitly handle cases where monitoring isn't LANGFUSE. The function implicitly returns None, which could cause errors when clients attempt to use the returned value as a decorator.
Missing docstring to explain the function's purpose and behavior.
def get_observe(): + """ + Returns the appropriate observe decorator based on the configured monitoring tool. + + Returns: + callable: The observe decorator if supported monitoring tool is configured + None: If no supported monitoring tool is configured + """ monitoring = get_base_config().monitoring_tool if monitoring == Observer.LANGFUSE: from langfuse.decorators import observe return observe + # For future monitoring tools, add additional handlers here + + # Return no-op decorator as fallback + return lambda *args, **kwargs: (lambda f: f)
🧹 Nitpick comments (1)
cognee/base_config.py (1)
11-11: Use specific type hint instead of 'object'The type hint for
monitoring_toolis very generic (object). Consider using the actual Observer enum type for better code clarity and IDE support.- monitoring_tool: object = Observer.LANGFUSE + monitoring_tool: Observer = Observer.LANGFUSE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cognee/api/v1/cognify/code_graph_pipeline.py(2 hunks)cognee/base_config.py(1 hunks)cognee/infrastructure/llm/gemini/adapter.py(1 hunks)cognee/infrastructure/llm/openai/adapter.py(2 hunks)cognee/modules/observability/get_observe.py(1 hunks)cognee/modules/observability/observers.py(1 hunks)cognee/shared/data_models.py(0 hunks)
💤 Files with no reviewable changes (1)
- cognee/shared/data_models.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
cognee/infrastructure/llm/openai/adapter.py (1)
cognee/modules/observability/get_observe.py (1)
get_observe(5-11)
cognee/base_config.py (2)
cognee/modules/observability/observers.py (1)
Observer(4-9)cognee/api/v1/config/config.py (1)
monitoring_tool(37-39)
cognee/infrastructure/llm/gemini/adapter.py (5)
cognee/shared/logging_utils.py (1)
get_logger(137-158)cognee/modules/observability/get_observe.py (1)
get_observe(5-11)cognee/exceptions/exceptions.py (1)
InvalidValueError(38-45)cognee/infrastructure/llm/prompts/read_query_prompt.py (1)
read_query_prompt(6-22)cognee/infrastructure/llm/rate_limiter.py (2)
rate_limit_async(220-243)sleep_and_retry_async(331-376)
cognee/modules/observability/get_observe.py (3)
cognee/base_config.py (1)
get_base_config(29-30)cognee/modules/observability/observers.py (1)
Observer(4-9)cognee/api/v1/config/config.py (1)
monitoring_tool(37-39)
cognee/api/v1/cognify/code_graph_pipeline.py (3)
cognee/shared/logging_utils.py (1)
get_logger(137-158)cognee/modules/observability/get_observe.py (1)
get_observe(5-11)cognee/shared/data_models.py (2)
KnowledgeGraph(33-39)KnowledgeGraph(63-67)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: End-to-End Tests / Run Telemetry Pipeline Test
- GitHub Check: End-to-End Tests / Server Start Test
- GitHub Check: End-to-End Tests / S3 Bucket Test
- GitHub Check: End-to-End Tests / Deletion Test
- GitHub Check: End-to-End Tests / Deduplication Test
- GitHub Check: Basic Tests / Run Unit Tests
- GitHub Check: Basic Tests / Run Simple Examples
- GitHub Check: Basic Tests / Run Basic Graph Tests
- GitHub Check: Basic Tests / Run Integration Tests
🔇 Additional comments (6)
cognee/infrastructure/llm/gemini/adapter.py (2)
6-7: Improved import organizationThe imports have been nicely reorganized to use the new centralized observability module.
17-17: Good usage of the centralized observability functionUsing
get_observe()centralizes the observability configuration logic, making the code cleaner and more maintainable than having conditional imports scattered throughout the codebase.cognee/modules/observability/observers.py (1)
4-9: Well-structured Observer enumThe Observer enum is well-structured as a string enum, allowing easy string comparison. This makes the code more maintainable by centralizing all supported monitoring tools in a single location.
cognee/base_config.py (1)
5-6: Import order and module selection look goodThe new import from the dedicated observability module is appropriate.
cognee/infrastructure/llm/openai/adapter.py (1)
5-6: Clean refactoring of observability importsThe code now uses the new abstracted observability pattern with
get_observe()instead of directly importing and conditionally handling monitoring tools. This simplifies the code and aligns with the PR's goal of abstracting logging tool integration.Also applies to: 18-18, 20-20
cognee/api/v1/cognify/code_graph_pipeline.py (1)
5-7: Good simplification of observability patternThe changes properly implement the new abstracted observability pattern by:
- Adding the import for
get_observe- Removing unnecessary imports from
cognee.shared.data_models(keeping onlyKnowledgeGraph)- Using
observe = get_observe()to centralize the decorator retrievalThis approach makes the codebase more maintainable and follows the single responsibility principle.
Also applies to: 14-14, 24-24
<!-- .github/pull_request_template.md --> ## Description <!-- Provide a clear description of the changes in this PR --> ## DCO Affirmation I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin. --------- Co-authored-by: Vasilije <[email protected]>
borisarzentar
deleted the
feature/cog-1675-abstract-logging-tool-integration
branch
Description
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.