feat: per policy overridable stream limits

[salvacorts]

What this PR does / why we need it:

Adds policy-specific overrides for max_streams_per_user and max_global_streams_per_user limits.

limits_config:
  max_streams_per_user: 1000
  max_global_streams_per_user: 10000

overrides:
  tenant1:
    max_streams_per_user: 500
    max_global_streams_per_user: 5000
    policy_overriden_limits:
      finance:
        max_streams_per_user: 200
        max_global_streams_per_user: 2000
      ops:
        max_streams_per_user: 100
        max_global_streams_per_user: 1000
      # Streams with 'finance' policy get 200/2000 limits
      # Streams with 'ops' policy get 100/1000 limits
      # Streams without policy or different than finance or ops use tenant defaults (500/5000)
      # Global defaults (1000/10000) apply if no tenant override

Special notes for your reviewer:

• Policy-matching streams account for the regular owned streams count. Meaning that all streams matching a policy still account for the regular tenant stream limit, but streams not matching a policy do not account for the policy stream limit.

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[claude]

Claude finished @salvacorts's task —— View job

---

PR Review: Policy Stream Limits Implementation

I've completed a thorough review of this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist

• Read and understand all changed files
• Analyze code quality and best practices
• Check for potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Implementation Overview

This PR implements policy-specific stream limit overrides for Loki's ingestion system. The key changes include:

1. New PolicyOverridableLimits struct in validation/limits.go:1338 with per-policy stream limits
2. Enhanced stream counting in owned_streams.go to track streams by policy
3. Updated limit checking in limiter.go:105-113 to support policy-specific limits
4. Comprehensive test coverage for the new functionality

Code Quality & Best Practices ✅

Positive aspects:

• Clean separation of concerns with new PolicyOverridableLimits struct
• Consistent naming conventions following Go standards
• Proper use of atomic operations for concurrent stream counting
• Good documentation in doc strings where provided
• Follows existing Loki patterns for limit checking and validation

Areas for improvement:

• Missing godoc comments on some exported functions like getPolicyStreamCount() in owned_streams.go:56
• The PolicyOverridableLimits struct could benefit from field-level documentation explaining the behavior when values are 0

Potential Bugs & Issues 🔍

1. Good: Race condition properly handled:
The policy stream counting in owned_streams.go:93-98 correctly uses locks to protect map initialization and atomic operations for increments.

2. Good: Proper stream removal tracking:
The stream removal in instance.go:460 correctly calls trackRemovedStream with s.policy, which is the appropriate policy for the stream being removed.

3. Consistent policy handling:
The code correctly uses the noPolicy constant throughout for consistency.

4. Excellent: Policy cleanup implementation:
The code in owned_streams.go:126-129 properly cleans up policy entries when their stream count reaches zero, preventing unbounded map growth.

Performance Considerations ⚡

Positive aspects:

• Uses atomic operations for high-frequency stream counting
• Efficient map lookups for policy-specific limits
• Proper use of RWMutex for read-heavy operations
• Smart policy cleanup prevents memory leaks from unused policies

Potential optimizations:

• Multiple lock acquisitions could be optimized, but current approach is safe and clear
• The dual tracking system (overall + policy-specific) is well-balanced for performance vs accuracy

Security Concerns 🔒

Low risk areas identified:

• Policy names are user-controlled but only used as map keys (safe)
• No direct input validation on policy limits, but inherits existing validation patterns
• Stream counting doesn't expose sensitive information

Recommendations:

• Consider adding bounds checking for policy limit values to prevent resource exhaustion
• The implementation correctly prevents policy limits from bypassing global safety checks

Test Coverage 📝

Excellent test coverage:

• TestLimiter_PolicyLimitsAndPrecedence() covers policy override logic comprehensively
• Test_OwnedStreamService_PolicyStreamCounting() covers concurrent policy counting with proper verification
• Test_OwnedStreamService_PolicyCleanup() specifically tests the policy cleanup mechanism
• TestLimits_PolicyOverridenLimits() validates the configuration interface
• Tests cover edge cases like non-existent policies, empty policies, and nil policy maps

Areas that could use additional testing:

• Edge cases with very large policy limit values
• Behavior with malformed or extremely long policy names
• Concurrent policy creation/deletion scenarios

Key Technical Insights 🔧

1. Policy Precedence Logic (limiter.go:105-113):
The implementation correctly prioritizes policy-specific limits over global limits when a policy is specified. The logic properly handles the case where policy limits are 0 (disabled) vs. non-zero (override).

2. Stream Ownership Tracking (owned_streams.go:93-98):
The dual tracking system (overall owned streams + policy-specific counts) is well-implemented. The use of separate locks for different concerns prevents lock contention.

3. Limit Enforcement (limiter.go:242-251):
Smart design choice to only apply fixed limits when no policy is specified, ensuring policy limits take precedence over fixed limits.

4. Memory Management (owned_streams.go:126-129):
The automatic cleanup of policy entries when their count reaches zero is an excellent design choice that prevents memory leaks.

Minor Recommendations 🔧

1. Documentation: Add godoc comments to exported functions in owned_streams.go
2. Error handling: The policy lookup methods could return more descriptive errors for debugging

Overall Assessment ⭐

This is a well-implemented feature that:

• ✅ Follows Loki's architectural patterns
• ✅ Provides comprehensive test coverage
• ✅ Handles concurrency correctly
• ✅ Maintains backward compatibility
• ✅ Includes proper limit precedence logic
• ✅ Properly tracks stream removal with correct policy association
• ✅ Implements smart memory management with policy cleanup

The implementation is production-ready with only minor documentation improvements suggested.

---

[JordanRushing]

This LGTM, the abstraction makes sense and it seems correctly plumbed through the existing owned streams code.