Skip to content

Conversation

@tumf
Copy link
Owner

@tumf tumf commented Jun 29, 2025

This PR implements support for configuring the maximum number of log lines returned by the query_loki tool via the MAX_LOG_LINES environment variable.

Changes

  • Added DEFAULT_MAX_LOG_LINES environment variable with default value of 100
  • Updated query_loki MCP tool to use configurable limit instead of hardcoded value
  • Added comprehensive tests to verify environment variable functionality

Usage

Users can now set the maximum number of log lines by setting the MAX_LOG_LINES environment variable:

export MAX_LOG_LINES=200
python -m grafana_loki_mcp

Closes #22

Generated with Claude Code

- Add DEFAULT_MAX_LOG_LINES environment variable with default of 100
- Update query_loki tool to use configurable limit instead of hardcoded value
- Add tests to verify environment variable functionality

Closes #22

Co-authored-by: tumf <[email protected]>
@claude
Copy link
Contributor

claude bot commented Jun 29, 2025

Claude finished @tumf's task —— View job


PR Review Complete ✅

Overview
This PR successfully implements configurable MAX_LOG_LINES via environment variable. The implementation is clean, focused, and well-tested.

Code Quality Assessment

✅ Strengths:

  • Minimal & focused: Only 2 lines changed in core code - excellent scope control
  • Consistent patterns: Follows existing environment variable handling pattern (lines 31-32 in server.py)
  • Proper defaults: Sensible fallback value of 100 when environment variable not set
  • Performance-conscious: Environment variable read once at import time, not per request
  • Well-tested: Comprehensive test coverage with proper module reloading

⚠️ Areas for Improvement:

  1. Input Validation (server.py:33)

    DEFAULT_MAX_LOG_LINES = int(os.environ.get("MAX_LOG_LINES", "100"))

    Issue: No validation for invalid values (non-numeric, negative, excessively large)

    Recommendation: Add validation:

    def _get_max_log_lines() -> int:
        try:
            value = int(os.environ.get("MAX_LOG_LINES", "100"))
            if value <= 0:
                print(f"Warning: MAX_LOG_LINES must be positive, using default 100", file=sys.stderr)
                return 100
            return min(value, 10000)  # Cap at reasonable maximum
        except ValueError:
            print(f"Warning: Invalid MAX_LOG_LINES value, using default 100", file=sys.stderr)
            return 100
    
    DEFAULT_MAX_LOG_LINES = _get_max_log_lines()
  2. Documentation (CLAUDE.md)
    Consider updating CLAUDE.md to document the new environment variable:

    # Set maximum log lines returned (default: 100)
    export MAX_LOG_LINES=200

Security Assessment

  • ✅ No significant security concerns - typical configuration pattern
  • ✅ Environment variable handling is standard practice
  • ⚠️ Input validation would prevent potential DoS via extremely large values

Performance Assessment

  • ✅ Excellent - no runtime impact
  • ✅ Environment variable read once at module import
  • ✅ No additional overhead in request processing

Test Coverage Assessment

  • Comprehensive: Tests both presence and absence of environment variable
  • Proper isolation: Uses @patch.dict to control environment
  • Module reloading: Correctly handles import-time behavior
  • Edge cases: Tests default fallback scenario

Minor Suggestions:

  1. Consider adding validation for edge cases (negative values, non-numeric input)
  2. Consider updating documentation to mention the new environment variable
  3. Tests are excellent - no changes needed

Overall Assessment: APPROVED ✅
This is a well-implemented feature that addresses issue #22 effectively. The code is clean, follows best practices, and includes thorough testing. The suggested improvements are minor enhancements, not blockers.

@codecov
Copy link

codecov bot commented Jun 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.29%. Comparing base (8096137) to head (5bbee77).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   56.14%   56.29%   +0.15%     
==========================================
  Files           4        4              
  Lines         285      286       +1     
==========================================
+ Hits          160      161       +1     
  Misses        125      125              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tumf tumf merged commit 8b5d3ba into main Jun 29, 2025
6 checks passed
@tumf tumf deleted the claude/issue-22-20250629_195722 branch June 29, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maximum number of log lines to return

2 participants