Skip to content

Commit c359d14

Browse files
TingluoHuangnikola-jokic
TingluoHuang
and
nikola-jokic
authored
Avoid nil point when config.Metrics is nil and expose all metrics if none are configured (#4101)
Co-authored-by: Nikola Jokic <jokicnikola07@gmail.com>
1 parent 9d8c59a commit c359d14

File tree

3 files changed

+315
-3
lines changed

3 files changed

+315
-3
lines changed

‎cmd/ghalistener/app/app.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ func New(config config.Config) (*App, error) {
6969
Repository: ghConfig.Repository,
7070
ServerAddr: config.MetricsAddr,
7171
ServerEndpoint: config.MetricsEndpoint,
72+
Metrics: config.Metrics,
7273
Logger: app.logger.WithName("metrics exporter"),
73-
Metrics: *config.Metrics,
7474
})
7575
}
7676

‎cmd/ghalistener/metrics/metrics.go

Lines changed: 137 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,148 @@ type ExporterConfig struct {
154154
ServerAddr string
155155
ServerEndpoint string
156156
Logger logr.Logger
157-
Metrics v1alpha1.MetricsConfig
157+
Metrics *v1alpha1.MetricsConfig
158+
}
159+
160+
var defaultMetrics = v1alpha1.MetricsConfig{
161+
Counters: map[string]*v1alpha1.CounterMetric{
162+
MetricStartedJobsTotal: {
163+
Labels: []string{
164+
labelKeyEnterprise,
165+
labelKeyOrganization,
166+
labelKeyRepository,
167+
labelKeyJobName,
168+
labelKeyEventName,
169+
},
170+
},
171+
MetricCompletedJobsTotal: {
172+
Labels: []string{
173+
labelKeyEnterprise,
174+
labelKeyOrganization,
175+
labelKeyRepository,
176+
labelKeyJobName,
177+
labelKeyEventName,
178+
labelKeyJobResult,
179+
},
180+
},
181+
},
182+
Gauges: map[string]*v1alpha1.GaugeMetric{
183+
MetricAssignedJobs: {
184+
Labels: []string{
185+
labelKeyEnterprise,
186+
labelKeyOrganization,
187+
labelKeyRepository,
188+
labelKeyRunnerScaleSetName,
189+
labelKeyRunnerScaleSetNamespace,
190+
},
191+
},
192+
MetricRunningJobs: {
193+
Labels: []string{
194+
labelKeyEnterprise,
195+
labelKeyOrganization,
196+
labelKeyRepository,
197+
labelKeyRunnerScaleSetName,
198+
labelKeyRunnerScaleSetNamespace,
199+
},
200+
},
201+
MetricRegisteredRunners: {
202+
Labels: []string{
203+
labelKeyEnterprise,
204+
labelKeyOrganization,
205+
labelKeyRepository,
206+
labelKeyRunnerScaleSetName,
207+
labelKeyRunnerScaleSetNamespace,
208+
},
209+
},
210+
MetricBusyRunners: {
211+
Labels: []string{
212+
labelKeyEnterprise,
213+
labelKeyOrganization,
214+
labelKeyRepository,
215+
labelKeyRunnerScaleSetName,
216+
labelKeyRunnerScaleSetNamespace,
217+
},
218+
},
219+
MetricMinRunners: {
220+
Labels: []string{
221+
labelKeyEnterprise,
222+
labelKeyOrganization,
223+
labelKeyRepository,
224+
labelKeyRunnerScaleSetName,
225+
labelKeyRunnerScaleSetNamespace,
226+
},
227+
},
228+
MetricMaxRunners: {
229+
Labels: []string{
230+
labelKeyEnterprise,
231+
labelKeyOrganization,
232+
labelKeyRepository,
233+
labelKeyRunnerScaleSetName,
234+
labelKeyRunnerScaleSetNamespace,
235+
},
236+
},
237+
MetricDesiredRunners: {
238+
Labels: []string{
239+
labelKeyEnterprise,
240+
labelKeyOrganization,
241+
labelKeyRepository,
242+
labelKeyRunnerScaleSetName,
243+
labelKeyRunnerScaleSetNamespace,
244+
},
245+
},
246+
MetricIdleRunners: {
247+
Labels: []string{
248+
labelKeyEnterprise,
249+
labelKeyOrganization,
250+
labelKeyRepository,
251+
labelKeyRunnerScaleSetName,
252+
labelKeyRunnerScaleSetNamespace,
253+
},
254+
},
255+
},
256+
Histograms: map[string]*v1alpha1.HistogramMetric{
257+
MetricJobStartupDurationSeconds: {
258+
Labels: []string{
259+
labelKeyEnterprise,
260+
labelKeyOrganization,
261+
labelKeyRepository,
262+
labelKeyJobName,
263+
labelKeyEventName,
264+
},
265+
Buckets: defaultRuntimeBuckets,
266+
},
267+
MetricJobExecutionDurationSeconds: {
268+
Labels: []string{
269+
labelKeyEnterprise,
270+
labelKeyOrganization,
271+
labelKeyRepository,
272+
labelKeyJobName,
273+
labelKeyEventName,
274+
labelKeyJobResult,
275+
},
276+
Buckets: defaultRuntimeBuckets,
277+
},
278+
},
279+
}
280+
281+
func (e *ExporterConfig) defaults() {
282+
if e.ServerAddr == "" {
283+
e.ServerAddr = ":8080"
284+
}
285+
if e.ServerEndpoint == "" {
286+
e.ServerEndpoint = "/metrics"
287+
}
288+
if e.Metrics == nil {
289+
defaultMetrics := defaultMetrics
290+
e.Metrics = &defaultMetrics
291+
}
158292
}
159293

160294
func NewExporter(config ExporterConfig) ServerExporter {
295+
config.defaults()
161296
reg := prometheus.NewRegistry()
162297

163-
metrics := installMetrics(config.Metrics, reg, config.Logger)
298+
metrics := installMetrics(*config.Metrics, reg, config.Logger)
164299

165300
mux := http.NewServeMux()
166301
mux.Handle(

‎cmd/ghalistener/metrics/metrics_test.go

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/go-logr/logr"
88
"github.com/prometheus/client_golang/prometheus"
99
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
1011
)
1112

1213
func TestInstallMetrics(t *testing.T) {
@@ -86,3 +87,179 @@ func TestInstallMetrics(t *testing.T) {
8687
assert.Equal(t, duration.config.Labels, metricsConfig.Histograms[MetricJobStartupDurationSeconds].Labels)
8788
assert.Equal(t, duration.config.Buckets, defaultRuntimeBuckets)
8889
}
90+
91+
func TestNewExporter(t *testing.T) {
92+
t.Run("with defaults metrics applied", func(t *testing.T) {
93+
config := ExporterConfig{
94+
ScaleSetName: "test-scale-set",
95+
ScaleSetNamespace: "test-namespace",
96+
Enterprise: "",
97+
Organization: "org",
98+
Repository: "repo",
99+
ServerAddr: ":6060",
100+
ServerEndpoint: "/metrics",
101+
Logger: logr.Discard(),
102+
Metrics: nil, // when metrics is nil, all default metrics should be registered
103+
}
104+
105+
exporter, ok := NewExporter(config).(*exporter)
106+
require.True(t, ok, "expected exporter to be of type *exporter")
107+
require.NotNil(t, exporter)
108+
109+
reg := prometheus.NewRegistry()
110+
wantMetrics := installMetrics(defaultMetrics, reg, config.Logger)
111+
112+
assert.Equal(t, len(wantMetrics.counters), len(exporter.counters))
113+
for k, v := range wantMetrics.counters {
114+
assert.Contains(t, exporter.counters, k)
115+
assert.Equal(t, v.config, exporter.counters[k].config)
116+
}
117+
118+
assert.Equal(t, len(wantMetrics.gauges), len(exporter.gauges))
119+
for k, v := range wantMetrics.gauges {
120+
assert.Contains(t, exporter.gauges, k)
121+
assert.Equal(t, v.config, exporter.gauges[k].config)
122+
}
123+
124+
assert.Equal(t, len(wantMetrics.histograms), len(exporter.histograms))
125+
for k, v := range wantMetrics.histograms {
126+
assert.Contains(t, exporter.histograms, k)
127+
assert.Equal(t, v.config, exporter.histograms[k].config)
128+
}
129+
130+
require.NotNil(t, exporter.srv)
131+
assert.Equal(t, config.ServerAddr, exporter.srv.Addr)
132+
})
133+
134+
t.Run("with default server URL", func(t *testing.T) {
135+
config := ExporterConfig{
136+
ScaleSetName: "test-scale-set",
137+
ScaleSetNamespace: "test-namespace",
138+
Enterprise: "",
139+
Organization: "org",
140+
Repository: "repo",
141+
ServerAddr: "", // empty ServerAddr should default to ":8080"
142+
ServerEndpoint: "",
143+
Logger: logr.Discard(),
144+
Metrics: nil, // when metrics is nil, all default metrics should be registered
145+
}
146+
147+
exporter, ok := NewExporter(config).(*exporter)
148+
require.True(t, ok, "expected exporter to be of type *exporter")
149+
require.NotNil(t, exporter)
150+
151+
reg := prometheus.NewRegistry()
152+
wantMetrics := installMetrics(defaultMetrics, reg, config.Logger)
153+
154+
assert.Equal(t, len(wantMetrics.counters), len(exporter.counters))
155+
for k, v := range wantMetrics.counters {
156+
assert.Contains(t, exporter.counters, k)
157+
assert.Equal(t, v.config, exporter.counters[k].config)
158+
}
159+
160+
assert.Equal(t, len(wantMetrics.gauges), len(exporter.gauges))
161+
for k, v := range wantMetrics.gauges {
162+
assert.Contains(t, exporter.gauges, k)
163+
assert.Equal(t, v.config, exporter.gauges[k].config)
164+
}
165+
166+
assert.Equal(t, len(wantMetrics.histograms), len(exporter.histograms))
167+
for k, v := range wantMetrics.histograms {
168+
assert.Contains(t, exporter.histograms, k)
169+
assert.Equal(t, v.config, exporter.histograms[k].config)
170+
}
171+
172+
require.NotNil(t, exporter.srv)
173+
assert.Equal(t, exporter.srv.Addr, ":8080")
174+
})
175+
176+
t.Run("with metrics configured", func(t *testing.T) {
177+
metricsConfig := v1alpha1.MetricsConfig{
178+
Counters: map[string]*v1alpha1.CounterMetric{
179+
MetricStartedJobsTotal: {
180+
Labels: []string{labelKeyRepository},
181+
},
182+
},
183+
Gauges: map[string]*v1alpha1.GaugeMetric{
184+
MetricAssignedJobs: {
185+
Labels: []string{labelKeyRepository},
186+
},
187+
},
188+
Histograms: map[string]*v1alpha1.HistogramMetric{
189+
MetricJobExecutionDurationSeconds: {
190+
Labels: []string{labelKeyRepository},
191+
Buckets: []float64{0.1, 1},
192+
},
193+
},
194+
}
195+
196+
config := ExporterConfig{
197+
ScaleSetName: "test-scale-set",
198+
ScaleSetNamespace: "test-namespace",
199+
Enterprise: "",
200+
Organization: "org",
201+
Repository: "repo",
202+
ServerAddr: ":6060",
203+
ServerEndpoint: "/metrics",
204+
Logger: logr.Discard(),
205+
Metrics: &metricsConfig,
206+
}
207+
208+
exporter, ok := NewExporter(config).(*exporter)
209+
require.True(t, ok, "expected exporter to be of type *exporter")
210+
require.NotNil(t, exporter)
211+
212+
reg := prometheus.NewRegistry()
213+
wantMetrics := installMetrics(metricsConfig, reg, config.Logger)
214+
215+
assert.Equal(t, len(wantMetrics.counters), len(exporter.counters))
216+
for k, v := range wantMetrics.counters {
217+
assert.Contains(t, exporter.counters, k)
218+
assert.Equal(t, v.config, exporter.counters[k].config)
219+
}
220+
221+
assert.Equal(t, len(wantMetrics.gauges), len(exporter.gauges))
222+
for k, v := range wantMetrics.gauges {
223+
assert.Contains(t, exporter.gauges, k)
224+
assert.Equal(t, v.config, exporter.gauges[k].config)
225+
}
226+
227+
assert.Equal(t, len(wantMetrics.histograms), len(exporter.histograms))
228+
for k, v := range wantMetrics.histograms {
229+
assert.Contains(t, exporter.histograms, k)
230+
assert.Equal(t, v.config, exporter.histograms[k].config)
231+
}
232+
233+
require.NotNil(t, exporter.srv)
234+
assert.Equal(t, config.ServerAddr, exporter.srv.Addr)
235+
})
236+
}
237+
238+
func TestExporterConfigDefaults(t *testing.T) {
239+
config := ExporterConfig{
240+
ScaleSetName: "test-scale-set",
241+
ScaleSetNamespace: "test-namespace",
242+
Enterprise: "",
243+
Organization: "org",
244+
Repository: "repo",
245+
ServerAddr: "",
246+
ServerEndpoint: "",
247+
Logger: logr.Discard(),
248+
Metrics: nil, // when metrics is nil, all default metrics should be registered
249+
}
250+
251+
config.defaults()
252+
want := ExporterConfig{
253+
ScaleSetName: "test-scale-set",
254+
ScaleSetNamespace: "test-namespace",
255+
Enterprise: "",
256+
Organization: "org",
257+
Repository: "repo",
258+
ServerAddr: ":8080", // default server address
259+
ServerEndpoint: "/metrics", // default server endpoint
260+
Logger: logr.Discard(),
261+
Metrics: &defaultMetrics, // when metrics is nil, all default metrics should be registered
262+
}
263+
264+
assert.Equal(t, want, config)
265+
}

0 commit comments

Comments
 (0)