Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use timeout flag for context cancelation #868

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use timeout flag for context cancelation #868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions client/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@
}

// GetStubStats fetches the stub_status metrics.
func (client *NginxClient) GetStubStats() (*StubStats, error) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

func (client *NginxClient) GetStubStats(ctx context.Context) (*StubStats, error) {

Check warning on line 50 in client/nginx.go

View check run for this annotation

Codecov / codecov/patch

client/nginx.go#L50 

Added line #L50 was not covered by tests
req, err := http.NewRequestWithContext(ctx, http.MethodGet, client.apiEndpoint, nil)
if err != nil {
return nil, fmt.Errorf("failed to create a get request: %w", err)
Expand Down
11 changes: 9 additions & 2 deletions collector/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package collector

import (
"context"
"log/slog"
"sync"
"time"

"github.com/nginxinc/nginx-prometheus-exporter/client"
"github.com/prometheus/client_golang/prometheus"
Expand All @@ -14,14 +16,16 @@
logger *slog.Logger
nginxClient *client.NginxClient
metrics map[string]*prometheus.Desc
timeout time.Duration
mutex sync.Mutex
}

// NewNginxCollector creates an NginxCollector.
func NewNginxCollector(nginxClient *client.NginxClient, namespace string, constLabels map[string]string, logger *slog.Logger) *NginxCollector {
func NewNginxCollector(nginxClient *client.NginxClient, namespace string, constLabels map[string]string, logger *slog.Logger, timeout time.Duration) *NginxCollector {

Check warning on line 24 in collector/nginx.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx.go#L24 

Added line #L24 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this function provide a sensible default for the timeout? Currently it breaks public API.

return &NginxCollector{
nginxClient: nginxClient,
logger: logger,
timeout: timeout,

Check warning on line 28 in collector/nginx.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx.go#L28 

Added line #L28 was not covered by tests
metrics: map[string]*prometheus.Desc{
"connections_active": newGlobalMetric(namespace, "connections_active", "Active client connections", constLabels),
"connections_accepted": newGlobalMetric(namespace, "connections_accepted", "Accepted client connections", constLabels),
Expand Down Expand Up @@ -50,7 +54,10 @@
c.mutex.Lock() // To protect metrics from concurrent collects
defer c.mutex.Unlock()

stats, err := c.nginxClient.GetStubStats()
ctx, cancel := context.WithTimeout(context.Background(), c.timeout)
defer cancel()

Check warning on line 58 in collector/nginx.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx.go#L57-L58 

Added lines #L57 - L58 were not covered by tests

stats, err := c.nginxClient.GetStubStats(ctx)

Check warning on line 60 in collector/nginx.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx.go#L60 

Added line #L60 was not covered by tests
if err != nil {
c.upMetric.Set(nginxDown)
ch <- c.upMetric
Expand Down
19 changes: 12 additions & 7 deletions collector/nginx_plus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"log/slog"
"strconv"
"sync"
"time"

plusclient "github.com/nginxinc/nginx-plus-go-client/v2/client"
"github.com/prometheus/client_golang/prometheus"
Expand All @@ -31,11 +32,11 @@

// NginxPlusCollector collects NGINX Plus metrics. It implements prometheus.Collector interface.
type NginxPlusCollector struct {
upMetric prometheus.Gauge
nginxClient *plusclient.NginxClient
logger *slog.Logger
upMetric prometheus.Gauge
cacheZoneMetrics map[string]*prometheus.Desc
workerMetrics map[string]*prometheus.Desc
nginxClient *plusclient.NginxClient
streamServerZoneMetrics map[string]*prometheus.Desc
streamZoneSyncMetrics map[string]*prometheus.Desc
streamUpstreamMetrics map[string]*prometheus.Desc
Expand All @@ -47,16 +48,17 @@
streamLimitConnectionMetrics map[string]*prometheus.Desc
upstreamServerMetrics map[string]*prometheus.Desc
upstreamMetrics map[string]*prometheus.Desc
streamUpstreamServerPeerLabels map[string][]string
serverZoneMetrics map[string]*prometheus.Desc
totalMetrics map[string]*prometheus.Desc
streamUpstreamServerPeerLabels map[string][]string
upstreamServerLabels map[string][]string
streamUpstreamServerLabels map[string][]string
serverZoneLabels map[string][]string
streamServerZoneLabels map[string][]string
upstreamServerPeerLabels map[string][]string
cacheZoneLabels map[string][]string
totalMetrics map[string]*prometheus.Desc
variableLabelNames VariableLabelNames
timeout time.Duration
variableLabelsMutex sync.RWMutex
mutex sync.Mutex
}
Expand Down Expand Up @@ -256,7 +258,7 @@
}

// NewNginxPlusCollector creates an NginxPlusCollector.
func NewNginxPlusCollector(nginxClient *plusclient.NginxClient, namespace string, variableLabelNames VariableLabelNames, constLabels map[string]string, logger *slog.Logger) *NginxPlusCollector {
func NewNginxPlusCollector(nginxClient *plusclient.NginxClient, namespace string, variableLabelNames VariableLabelNames, constLabels map[string]string, logger *slog.Logger, timeout time.Duration) *NginxPlusCollector {

Check warning on line 261 in collector/nginx_plus.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx_plus.go#L261 

Added line #L261 was not covered by tests
upstreamServerVariableLabelNames := variableLabelNames.UpstreamServerVariableLabelNames
streamUpstreamServerVariableLabelNames := variableLabelNames.StreamUpstreamServerVariableLabelNames

Expand All @@ -273,6 +275,7 @@
cacheZoneLabels: make(map[string][]string),
nginxClient: nginxClient,
logger: logger,
timeout: timeout,

Check warning on line 278 in collector/nginx_plus.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx_plus.go#L278 

Added line #L278 was not covered by tests
totalMetrics: map[string]*prometheus.Desc{
"connections_accepted": newGlobalMetric(namespace, "connections_accepted", "Accepted client connections", constLabels),
"connections_dropped": newGlobalMetric(namespace, "connections_dropped", "Dropped client connections", constLabels),
Expand Down Expand Up @@ -623,8 +626,10 @@
c.mutex.Lock() // To protect metrics from concurrent collects
defer c.mutex.Unlock()

// FIXME: https://github.com/nginxinc/nginx-prometheus-exporter/issues/858
stats, err := c.nginxClient.GetStats(context.TODO())
ctx, cancel := context.WithTimeout(context.Background(), c.timeout)
defer cancel()

Check warning on line 630 in collector/nginx_plus.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx_plus.go#L629-L630 

Added lines #L629 - L630 were not covered by tests

stats, err := c.nginxClient.GetStats(ctx)

Check warning on line 632 in collector/nginx_plus.go

View check run for this annotation

Codecov / codecov/patch

collector/nginx_plus.go#L632 

Added line #L632 was not covered by tests
if err != nil {
c.upMetric.Set(nginxDown)
ch <- c.upMetric
Expand Down
9 changes: 3 additions & 6 deletions exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,7 @@
_ = srv.Shutdown(srvCtx)
}

func registerCollector(logger *slog.Logger, transport *http.Transport,
addr string, labels map[string]string,
) {
func registerCollector(logger *slog.Logger, transport *http.Transport, addr string, labels map[string]string) {

Check warning on line 223 in exporter.go

View check run for this annotation

Codecov / codecov/patch

exporter.go#L223 

Added line #L223 was not covered by tests
if strings.HasPrefix(addr, "unix:") {
socketPath, requestPath, err := parseUnixSocketAddress(addr)
if err != nil {
Expand All @@ -239,7 +237,6 @@
userAgent := fmt.Sprintf("NGINX-Prometheus-Exporter/v%v", common_version.Version)

httpClient := &http.Client{
Timeout: *timeout,
Transport: &userAgentRoundTripper{
agent: userAgent,
rt: transport,
Expand All @@ -253,10 +250,10 @@
os.Exit(1)
}
variableLabelNames := collector.NewVariableLabelNames(nil, nil, nil, nil, nil, nil, nil)
prometheus.MustRegister(collector.NewNginxPlusCollector(plusClient, "nginxplus", variableLabelNames, labels, logger))
prometheus.MustRegister(collector.NewNginxPlusCollector(plusClient, "nginxplus", variableLabelNames, labels, logger, *timeout))

Check warning on line 253 in exporter.go

View check run for this annotation

Codecov / codecov/patch

exporter.go#L253 

Added line #L253 was not covered by tests
} else {
ossClient := client.NewNginxClient(httpClient, addr)
prometheus.MustRegister(collector.NewNginxCollector(ossClient, "nginx", labels, logger))
prometheus.MustRegister(collector.NewNginxCollector(ossClient, "nginx", labels, logger, *timeout))

Check warning on line 256 in exporter.go

View check run for this annotation

Codecov / codecov/patch

exporter.go#L256 

Added line #L256 was not covered by tests
}
}

Expand Down