Skip to content

Conversation

@akshayutture-augment
Copy link

…are (#76186)

* Plugins: Chore: Renamed instrumentation middleware to metrics middleware

* Removed repeated logger attributes in middleware and contextual logger

* renamed loggerParams to logParams

* PR review suggestion

* Add contextual logger middleware

* Removed unused params from logRequest

* Removed unwanted changes

* Safer FromContext method

* Removed traceID from logParams
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@akshayutture-augment
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Confidence Score: 5/5

  • This PR is safe to merge with no issues found
  • Well-architected refactoring with proper separation of concerns. All changes maintain backward compatibility, tests were updated appropriately, and the new FromContext method has proper fallback handling. The middleware order is correct and the contextual logger is positioned before the logger middleware to ensure context is available.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
pkg/plugins/log/ifaces.go 5/5 Updated interface parameters from interface{} to any and added FromContext method to Logger interface
pkg/plugins/log/logger.go 5/5 Implemented FromContext method for grafanaInfraLogWrapper with proper type assertion fallback
pkg/plugins/log/fake.go 5/5 Added FromContext method to TestLogger for test compatibility
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go 5/5 Renamed from instrumentation_middleware.go, removed contextual logging logic which was moved to separate middleware
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go 5/5 Renamed from instrumentation_middleware_test.go, updated to use new newMetricsMiddleware constructor
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go 5/5 New middleware extracted from MetricsMiddleware to add contextual logging attributes to request context
pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go 5/5 Simplified to use FromContext for retrieving contextual logger instead of manually building log parameters
pkg/services/pluginsintegration/pluginsintegration.go 5/5 Updated middleware stack: renamed NewInstrumentationMiddleware to NewMetricsMiddleware and added NewContextualLoggerMiddleware

Sequence Diagram

sequenceDiagram
    participant Client
    participant TracingMW as TracingMiddleware
    participant MetricsMW as MetricsMiddleware
    participant ContextualLoggerMW as ContextualLoggerMiddleware
    participant LoggerMW as LoggerMiddleware
    participant Plugin as Plugin Backend

    Client->>TracingMW: QueryData(ctx, req)
    TracingMW->>MetricsMW: QueryData(ctx, req)
    Note over MetricsMW: Track request metrics<br/>(count, duration, size)
    MetricsMW->>ContextualLoggerMW: QueryData(ctx, req)
    Note over ContextualLoggerMW: Add contextual attributes to ctx<br/>(endpoint, pluginId, dsName, uname)
    ContextualLoggerMW->>LoggerMW: QueryData(enriched_ctx, req)
    Note over LoggerMW: Extract logger from context<br/>using FromContext()
    LoggerMW->>Plugin: QueryData(enriched_ctx, req)
    Plugin-->>LoggerMW: response
    Note over LoggerMW: Log request completion<br/>with contextual info
    LoggerMW-->>ContextualLoggerMW: response
    ContextualLoggerMW-->>MetricsMW: response
    Note over MetricsMW: Record metrics with labels
    MetricsMW-->>TracingMW: response
    TracingMW-->>Client: response
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

2 participants