Document on how to configure HTTP client timeouts (#218)

* chore: Use client timeout for browser tabs. If end user uses a bigger timeouts for loading heavier panels, they should be propagated to browser tabs as well * docs: Add a section about advanced HTTP client settings in README * fix: Ensure to populate client opts even when JSONData is nil * Fix data races in unit tests --------- Signed-off-by: Mahendra Paipuri <mahendra.paipuri@gmail.com>
mahendrapaipuri · Dec 24, 2024 · 6dc4a16 · 6dc4a16
1 parent 9bd8139
commit 6dc4a16
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 23 deletions.
diff --git a/pkg/plugin/app.go b/pkg/plugin/app.go
@@ -72,13 +72,6 @@ func NewDashboardReporterApp(ctx context.Context, settings backend.AppInstanceSe
 		return nil, fmt.Errorf("error loading config: %w", err)
 	}
 
-	// Validate plugin config
-	if err := app.conf.Validate(); err != nil {
-		app.ctxLogger.Error("error config validation", "err", err)
-
-		return nil, fmt.Errorf("error config validation: %w", err)
-	}
-
 	app.ctxLogger.Info("starting plugin with initial config: " + app.conf.String())
 
 	// Make a new HTTP client

diff --git a/pkg/plugin/config/settings.go b/pkg/plugin/config/settings.go
@@ -171,6 +171,8 @@ func Load(ctx context.Context, settings backend.AppInstanceSettings) (Config, er
 		},
 	}
 
+	var err error
+
 	// Fetch token, if configured in SecureJSONData
 	if settings.DecryptedSecureJSONData != nil {
 		if saToken, ok := settings.DecryptedSecureJSONData[SaToken]; ok && saToken != "" {
@@ -179,14 +181,10 @@ func Load(ctx context.Context, settings backend.AppInstanceSettings) (Config, er
 	}
 
 	// Update plugin settings defaults
-	if settings.JSONData == nil || string(settings.JSONData) == "null" {
-		return config, nil
-	}
-
-	var err error
-
-	if err = json.Unmarshal(settings.JSONData, &config); err != nil { //nolint:musttag
-		return Config{}, err
+	if settings.JSONData != nil && string(settings.JSONData) != "null" {
+		if err = json.Unmarshal(settings.JSONData, &config); err != nil { //nolint:musttag
+			return Config{}, err
+		}
 	}
 
 	// Override provisioned config from env vars, if set

diff --git a/pkg/plugin/dashboard/data.go b/pkg/plugin/dashboard/data.go
@@ -24,7 +24,7 @@ func (d *Dashboard) PanelCSV(_ context.Context, p Panel) (CSVData, error) {
 	tab := d.chromeInstance.NewTab(d.logger, d.conf)
 	// Set a timeout for the tab
 	// Fail-safe for newer Grafana versions, if css has been changed.
-	tab.WithTimeout(60 * time.Second)
+	tab.WithTimeout(d.conf.HTTPClientOptions.Timeouts.Timeout)
 	defer tab.Close(d.logger)
 
 	headers := make(map[string]any)

diff --git a/pkg/plugin/dashboard/panels.go b/pkg/plugin/dashboard/panels.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"math"
-	"time"
 
 	"github.com/chromedp/cdproto/runtime"
 	"github.com/chromedp/chromedp"
@@ -130,7 +129,7 @@ func (d *Dashboard) panelData(_ context.Context) ([]interface{}, error) {
 
 	// Create a new tab
 	tab := d.chromeInstance.NewTab(d.logger, d.conf)
-	tab.WithTimeout(60 * time.Second)
+	tab.WithTimeout(2 * d.conf.HTTPClientOptions.Timeouts.Timeout)
 	defer tab.Close(d.logger)
 
 	headers := make(map[string]any)

diff --git a/pkg/plugin/dashboard/panels_test.go b/pkg/plugin/dashboard/panels_test.go
@@ -9,16 +9,20 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
+	"github.com/grafana/grafana-plugin-sdk-go/backend/httpclient"
 	"github.com/grafana/grafana-plugin-sdk-go/backend/log"
 	"github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/chrome"
 	"github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/config"
 	"github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/worker"
 	. "github.com/smartystreets/goconvey/convey"
 )
 
+var muLock sync.RWMutex
+
 func TestDashboardFetchWithLocalChrome(t *testing.T) {
 	var execPath string
 
@@ -73,8 +77,10 @@ func TestDashboardFetchWithLocalChrome(t *testing.T) {
 		requestCookie := ""
 
 		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			muLock.Lock()
 			requestURI = append(requestURI, r.RequestURI)
 			requestCookie = r.Header.Get(backend.CookiesHeaderName)
+			muLock.Unlock()
 
 			if _, err := w.Write(data); err != nil {
 				http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -86,8 +92,9 @@ func TestDashboardFetchWithLocalChrome(t *testing.T) {
 
 		Convey("When using the panels fetcher", func() {
 			conf := config.Config{
-				Layout:        "simple",
-				DashboardMode: "default",
+				Layout:            "simple",
+				DashboardMode:     "default",
+				HTTPClientOptions: httpclient.Options{Timeouts: &httpclient.DefaultTimeoutOptions},
 			}
 
 			ctx := context.Background()
@@ -173,8 +180,10 @@ func TestDashboardFetchWithRemoteChrome(t *testing.T) {
 		requestCookie := ""
 
 		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			muLock.Lock()
 			requestURI = append(requestURI, r.RequestURI)
 			requestCookie = r.Header.Get(backend.CookiesHeaderName)
+			muLock.Unlock()
 
 			if _, err := w.Write(data); err != nil {
 				http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -186,8 +195,9 @@ func TestDashboardFetchWithRemoteChrome(t *testing.T) {
 
 		Convey("When using the Grafana httpClient", func() {
 			conf := config.Config{
-				Layout:        "simple",
-				DashboardMode: "default",
+				Layout:            "simple",
+				DashboardMode:     "default",
+				HTTPClientOptions: httpclient.Options{Timeouts: &httpclient.DefaultTimeoutOptions},
 			}
 
 			ctx := context.Background()

diff --git a/src/README.md b/src/README.md
@@ -210,7 +210,7 @@ a new service token can be generated and configured with the plugin.
 
 This config section allows to configure report related settings.
 
-- `file:theme; env:GF_REPORTER_PLUGIN_REPORT_THEME; ui: Theme`: Theme of the panels in
+- `file:theme; env:GF_REPORTER_PLUGIN_REPORT_THEME; ui:Theme`: Theme of the panels in
   the report.
 
 - `file:layout; env:GF_REPORTER_PLUGIN_REPORT_LAYOUT; ui:Layout`: Layout of the report.
@@ -301,6 +301,9 @@ The following configuration settings allow more control over plugin's functional
 will get the Grafana's data path based on its own executable path. If the existing provisioned
 configs have this parameter set, it will be ignored while loading the plugin's configuration.
 
+More advanced settings on HTTP client can be configured using provisioned config file which are
+presented in [Advanced Settings](#advanced-settings).
+
 #### Overriding global report settings
 
 Although configuration settings can only be modified by users with `Admin` role for whole `Org`
@@ -495,6 +498,80 @@ Here are the example reports that are generated out of the test dashboards
 - [Report with landscape orientation, grid layout and full dashboard mode](https://github.com/mahendrapaipuri/grafana-dashboard-reporter-app/blob/main/docs/reports/report_landscape_grid_full.pdf)
 - [Report with portrait orientation, grid layout and default dashboard mode](https://github.com/mahendrapaipuri/grafana-dashboard-reporter-app/blob/main/docs/reports/report_portrait_grid_default.pdf)
 
+## Advanced Settings
+
+The plugin makes API requests to Grafana to fetch PNGs of individual panels in a dashboard.
+If a wider time window is selected in the dashboard during report generation, API requests
+might need a bigger timeout for the panel data to load in its entirety. By default, a timeout
+of 30 seconds is used in the HTTP client. If a bigger timeout is needed for a particular use
+case, it can be done using provisioned config file. A basic provisioned config file with
+to set timeout to 120 seconds can be defined as follows:
+
+```yaml
+apiVersion: 1
+
+apps:
+  - type: mahendrapaipuri-dashboardreporter-app
+    org_id: 1
+    org_name: Main Org.
+    disabled: false
+    
+    jsonData:
+      # HTTP Client timeout in seconds
+      #
+      timeout: 120
+```
+
+Along with timeout, there are several other HTTP client options can be set through
+provisioned config file which are shown below along with their default values:
+
+```yaml
+apiVersion: 1
+
+apps:
+  - type: mahendrapaipuri-dashboardreporter-app
+    org_id: 1
+    org_name: Main Org.
+    disabled: false
+    
+    jsonData:
+      # HTTP Client timeout in seconds
+      #
+      timeout: 30
+
+      # HTTP Client dial timeout in seconds
+      #
+      dialTimeout: 10
+
+      # HTTP Keep Alive timeout in seconds
+      #
+      httpKeepAlive: 30
+
+      # HTTP TLS handshake timeout in seconds
+      #
+      httpTLSHandshakeTimeout: 10
+
+      # HTTP idle connection timeout in seconds
+      #
+      httpIdleConnTimeout: 90
+
+      # HTTP max connections per host
+      #
+      httpMaxConnsPerHost: 0
+
+      # HTTP max idle connections
+      #
+      httpMaxIdleConns: 100
+
+      # HTTP max idle connections per host
+      #
+      httpMaxIdleConnsPerHost: 100
+```
+
+> [!NOTE]
+> These settings can be configured only through provisioned config file and it is not
+possible to set them using either environment variables or Grafana UI.
+
 ## Troubleshooting
 
 - When TLS is enabled on Grafana server, `grafana-image-renderer` tends to throw