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.

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

Telemetry Job #4896

Merged
merged 29 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
838c94a
Add base telemetry job
shaun-nx Jan 5, 2024
ca9b9f7
Ensure only leader pod reports data
shaun-nx Jan 9, 2024
5e3d349
Allow deployments to opt-out of telemetry collection
shaun-nx Jan 10, 2024
a9224db
Add log line for when telemetry is collected
shaun-nx Jan 10, 2024
54b896b
gofumpt files
shaun-nx Jan 10, 2024
8011a08
Revert deployment yaml and fake manager
shaun-nx Jan 10, 2024
079a906
Fix nginx version assignment
shaun-nx Jan 10, 2024
3a69f51
Merge branch 'main' into feat/telemetry
shaun-nx Jan 10, 2024
82a91a6
Resolve lint issues
shaun-nx Jan 10, 2024
ff7515e
Placeholder for telemetry collector
jjngx Jan 22, 2024
9448ae9
Simplify telemetry reporting flags
jjngx Jan 23, 2024
dc634ac
Limit reporting period to min 1m
jjngx Jan 23, 2024
3fa1756
Set min reporting period to 1h
jjngx Jan 24, 2024
c78e3fd
Merge branch 'main' into feat/telemetry
jjngx Jan 24, 2024
922bbe6
Use temp exporter for sending data
jjngx Jan 24, 2024
da52eed
Merge branch 'main' into feat/telemetry
jjngx Jan 24, 2024
2b8e4ad
Merge branch 'main' into feat/telemetry
jjngx Jan 25, 2024
301ed1e
Merge branch 'main' into feat/telemetry
jjngx Jan 26, 2024
cf31f97
Return fake nginx version
jjngx Jan 29, 2024
e249b56
Revert nginx version check changes
shaun-nx Jan 31, 2024
22ca1e8
Merge branch 'main' into feat/telemetry
shaun-nx Jan 31, 2024
37bf9fe
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
e248aa9
Set min reporting time to 1m
jjngx Feb 6, 2024
ec8678d
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
a56bc49
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
2cae174
Set default reporting period and add unit test for new telemetry coll…
shaun-nx Feb 6, 2024
d401d49
Add telemetry reporting flag to helm values
shaun-nx Feb 6, 2024
1d04236
Fix telemetry unit test
shaun-nx Feb 6, 2024
cebd70c
Merge branch 'main' into feat/telemetry
shaun-nx Feb 6, 2024
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
18 changes: 12 additions & 6 deletions cmd/nginx-ingress/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@
import (
"flag"
"fmt"
"github.com/golang/glog"

Check failure on line 6 in cmd/nginx-ingress/flags.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
api_v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/validation"
"net"
"os"
"regexp"
"strconv"
"strings"

"github.com/golang/glog"
api_v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/validation"
)

const (
dynamicSSLReloadParam = "ssl-dynamic-reload"
dynamicSSLReloadParam = "ssl-dynamic-reload"
enableTelemetryReportingParam = "enable-telemetry-reporting"
telemetryReportingPeriodParam = "telemetry-reporting-period"
defaultTelemetryReportingPeriod = "24h"
)

var (
Expand Down Expand Up @@ -201,6 +203,10 @@

enableDynamicSSLReload = flag.Bool(dynamicSSLReloadParam, true, "Enable reloading of SSL Certificates without restarting the NGINX process.")

enableTelemetryReporting = flag.Bool(enableTelemetryReportingParam, true, "Enable gathering and reporting of product related telemetry.")

telemetryReportingPeriod = flag.String(telemetryReportingPeriodParam, defaultTelemetryReportingPeriod, "Period at which product telemetry is reported.")

startupCheckFn func() error
)

Expand Down
15 changes: 11 additions & 4 deletions cmd/nginx-ingress/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@
)

// Injected during build
var version string
var (
version string
)

const (
nginxVersionLabel = "app.nginx.org/version"
Expand Down Expand Up @@ -199,6 +201,8 @@
ExternalDNSEnabled: *enableExternalDNS,
IsIPV6Disabled: *disableIPV6,
WatchNamespaceLabel: *watchNamespaceLabel,
TelemetryReportPeriod: *telemetryReportingPeriod,
EnableTelemetryReporting: *enableTelemetryReporting,

Check warning on line 205 in cmd/nginx-ingress/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/nginx-ingress/main.go#L204-L205

Added lines #L204 - L205 were not covered by tests
}

lbc := k8s.NewLoadBalancerController(lbcInput)
Expand Down Expand Up @@ -790,9 +794,12 @@
}

func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxVersion, appProtectVersion string, maxRetries int, waitTime time.Duration) {
nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n")
replacer := strings.NewReplacer(" ", "-", "(", "", ")", "")
nginxVer = replacer.Replace(nginxVer)
var nginxVer string
if nginxVersion != "" {
nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n")
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved
replacer := strings.NewReplacer(" ", "-", "(", "", ")", "")
nginxVer = replacer.Replace(nginxVer)

Check failure on line 801 in cmd/nginx-ingress/main.go

View workflow job for this annotation

GitHub Actions / Lint

ineffectual assignment to nginxVer (ineffassign)
}

Check warning on line 802 in cmd/nginx-ingress/main.go

View check run for this annotation

Codecov / codecov/patch

cmd/nginx-ingress/main.go#L797-L802

Added lines #L797 - L802 were not covered by tests
podUpdated := false

for i := 0; (i < maxRetries || maxRetries == 0) && !podUpdated; i++ {
Expand Down
6 changes: 3 additions & 3 deletions deployments/deployment/nginx-plus-ingress.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: nginx-ingress
namespace: nginx-ingress
spec:
replicas: 1
replicas: 2
selector:
matchLabels:
app: nginx-ingress
Expand Down Expand Up @@ -33,7 +33,7 @@ spec:
# - name: nginx-log
# emptyDir: {}
containers:
- image: nginx-plus-ingress:3.4.0
- image: nginx-plus-ingress:telemetry
imagePullPolicy: IfNotPresent
name: nginx-plus-ingress
ports:
Expand Down Expand Up @@ -96,7 +96,7 @@ spec:
#- -enable-external-dns
#- -enable-app-protect
#- -enable-app-protect-dos
#- -v=3 # Enables extensive logging. Useful for troubleshooting.
- -v=1 # Enables extensive logging. Useful for troubleshooting.
#- -report-ingress-status
#- -external-service=nginx-ingress
#- -enable-prometheus-metrics
Expand Down
37 changes: 37 additions & 0 deletions internal/k8s/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import (
"context"
"fmt"
"github.com/nginxinc/kubernetes-ingress/internal/telemetry"

Check failure on line 22 in internal/k8s/controller.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
"net"
"strconv"
"strings"
Expand Down Expand Up @@ -161,6 +162,8 @@
enableBatchReload bool
isIPV6Disabled bool
namespaceWatcherController cache.Controller
telemetryReporter telemetry.Reporter
telemetryChan chan struct{}
}

var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc
Expand Down Expand Up @@ -206,6 +209,8 @@
ExternalDNSEnabled bool
IsIPV6Disabled bool
WatchNamespaceLabel string
TelemetryReportPeriod string
EnableTelemetryReporting bool
}

// NewLoadBalancerController creates a controller
Expand Down Expand Up @@ -271,6 +276,24 @@
lbc.externalDNSController = ed_controller.NewController(ed_controller.BuildOpts(context.TODO(), lbc.namespaceList, lbc.recorder, lbc.confClient, input.ResyncPeriod, isDynamicNs))
}

if input.EnableTelemetryReporting {
// Placeholder exporter.
exporter := &telemetry.StdOutExporter{}
lbc.telemetryChan = make(chan struct{})
period, err := time.ParseDuration(input.TelemetryReportPeriod)

Check failure on line 284 in internal/k8s/controller.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
if err != nil {
glog.Fatalf("Error parsing duration for telemetry: %v", err)
}

Check warning on line 287 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L279-L287

Added lines #L279 - L287 were not covered by tests

config := telemetry.TraceTelemetryReporterConfig{
Exporter: exporter,
ReportingPeriod: period,
Data: telemetry.Data{},
}
lbc.telemetryReporter = telemetry.NewTelemetryReporter(config)

Check warning on line 294 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L289-L294

Added lines #L289 - L294 were not covered by tests
}

glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass)

lbc.namespacedInformers = make(map[string]*namespacedInformer)
Expand Down Expand Up @@ -683,10 +706,24 @@
if lbc.externalDNSController != nil {
go lbc.externalDNSController.Run(lbc.ctx.Done())
}

if lbc.leaderElector != nil {
go lbc.leaderElector.Run(lbc.ctx)
}

if lbc.telemetryReporter != nil {
go func(ctx context.Context) {
glog.V(1).Info("-- Checking if leader is set --")
select {
case <-lbc.telemetryChan:
lbc.telemetryReporter.Start(lbc.ctx)
case <-ctx.Done():
glog.V(1).Info("-- DONE Reporting Telemetry --")
return

Check warning on line 722 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L714-L722

Added lines #L714 - L722 were not covered by tests
}
}(lbc.ctx)
}

for _, nif := range lbc.namespacedInformers {
nif.start()
}
Expand Down
4 changes: 4 additions & 0 deletions internal/k8s/leader.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
return leaderelection.LeaderCallbacks{
OnStartedLeading: func(ctx context.Context) {
glog.V(3).Info("started leading")
// Closing this channel allows the leader to start the telemetry reporting process
if lbc.telemetryChan != nil {
close(lbc.telemetryChan)
}

Check warning on line 64 in internal/k8s/leader.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/leader.go#L61-L64

Added lines #L61 - L64 were not covered by tests
if lbc.reportIngressStatus {
ingresses := lbc.configuration.GetResourcesWithFilter(resourceFilter{Ingresses: true})

Expand Down
4 changes: 3 additions & 1 deletion internal/nginx/fake_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@
// Version provides a fake implementation of Version.
func (*FakeManager) Version() Version {
glog.V(3).Info("Printing nginx version")
return Version{}
return Version{
raw: "nginx/1.25.1 (nginx-plus-r30-p1)",
}

Check warning on line 108 in internal/nginx/fake_manager.go

View check run for this annotation

Codecov / codecov/patch

internal/nginx/fake_manager.go#L106-L108

Added lines #L106 - L108 were not covered by tests
}

// Start provides a fake implementation of Start.
Expand Down
25 changes: 25 additions & 0 deletions internal/telemetry/exporter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package telemetry

Check warning on line 1 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

package-comments: should have a package comment (revive)

import (
"context"

Check failure on line 4 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
"github.com/golang/glog"
)

type Data struct {

Check failure on line 8 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
}

type Exporter interface {

Check warning on line 11 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported type Exporter should have comment or be unexported (revive)
Export(ctx context.Context, data Data) error
}

type StdOutExporter struct {

Check failure on line 15 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
}

func NewStdOutExporter() *StdOutExporter {

Check warning on line 18 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported function NewStdOutExporter should have comment or be unexported (revive)
return &StdOutExporter{}
}

func (s *StdOutExporter) Export(_ context.Context, data Data) error {

Check warning on line 22 in internal/telemetry/exporter.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported method StdOutExporter.Export should have comment or be unexported (revive)
glog.V(1).Infof("Exporting data %v", data)
return nil
}
18 changes: 18 additions & 0 deletions internal/telemetry/exporter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package telemetry

import (
"context"
"testing"
)

func TestExportData(t *testing.T) {
t.Parallel()

exporter := NewStdOutExporter()

err := exporter.Export(context.Background(), Data{})

Check failure on line 14 in internal/telemetry/exporter_test.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
if err != nil {
t.Fatalf("Expeceted no error, but got %s", err.Error())
jjngx marked this conversation as resolved.
Show resolved Hide resolved
}
}
55 changes: 55 additions & 0 deletions internal/telemetry/telemetry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package telemetry

import (
"context"

Check failure on line 4 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
"github.com/golang/glog"
"k8s.io/apimachinery/pkg/util/wait"
"time"

Check failure on line 7 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
)

const (
jitterFactor = 0.1
sliding = true
)

type Reporter interface {

Check warning on line 15 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported type Reporter should have comment or be unexported (revive)
Start(ctx context.Context)
}

type TraceTelemetryReporterConfig struct {

Check warning on line 19 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported type TraceTelemetryReporterConfig should have comment or be unexported (revive)
Data Data
Exporter Exporter
ReportingPeriod time.Duration
}

type TraceTelemetryReporter struct {

Check warning on line 25 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported type TraceTelemetryReporter should have comment or be unexported (revive)
config TraceTelemetryReporterConfig
}

func NewTelemetryReporter(config TraceTelemetryReporterConfig) *TraceTelemetryReporter {

Check warning on line 29 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported function NewTelemetryReporter should have comment or be unexported (revive)
return &TraceTelemetryReporter{
config: config,
}

Check warning on line 32 in internal/telemetry/telemetry.go

View check run for this annotation

Codecov / codecov/patch

internal/telemetry/telemetry.go#L29-L32

Added lines #L29 - L32 were not covered by tests
}

func (t *TraceTelemetryReporter) Start(ctx context.Context) {

Check warning on line 35 in internal/telemetry/telemetry.go

View workflow job for this annotation

GitHub Actions / Lint

exported: exported method TraceTelemetryReporter.Start should have comment or be unexported (revive)
wait.JitterUntilWithContext(ctx, t.report, t.config.ReportingPeriod, jitterFactor, sliding)

Check warning on line 36 in internal/telemetry/telemetry.go

View check run for this annotation

Codecov / codecov/patch

internal/telemetry/telemetry.go#L35-L36

Added lines #L35 - L36 were not covered by tests
}

func (t *TraceTelemetryReporter) report(ctx context.Context) {
// Gather data here
t.setVirtualServerCount()
t.setTransportServerCount()

if err := t.config.Exporter.Export(ctx, t.config.Data); err != nil {
glog.Errorf("Error exporting telemetry data: %v", err)
}

Check warning on line 46 in internal/telemetry/telemetry.go

View check run for this annotation

Codecov / codecov/patch

internal/telemetry/telemetry.go#L39-L46

Added lines #L39 - L46 were not covered by tests
}

func (t *TraceTelemetryReporter) setVirtualServerCount() {
// Placeholder function

Check warning on line 50 in internal/telemetry/telemetry.go

View check run for this annotation

Codecov / codecov/patch

internal/telemetry/telemetry.go#L49-L50

Added lines #L49 - L50 were not covered by tests
}

func (t *TraceTelemetryReporter) setTransportServerCount() {
// Placeholder function

Check warning on line 54 in internal/telemetry/telemetry.go

View check run for this annotation

Codecov / codecov/patch

internal/telemetry/telemetry.go#L53-L54

Added lines #L53 - L54 were not covered by tests
}
30 changes: 30 additions & 0 deletions internal/telemetry/telemetry_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package telemetry

import (
"context"
"reflect"
"testing"
)

type MockTelemetryReport struct {
data Data
}

func (m *MockTelemetryReport) Start(_ context.Context) {
m.data = Data{}
}

func TestCollectData(t *testing.T) {
t.Parallel()

ctx := context.Background()
mtr := &MockTelemetryReport{
Data{},
}
expectedData := Data{}
mtr.Start(ctx)

if !reflect.DeepEqual(mtr.data, expectedData) {
t.Fatalf("expected %v, but got %v", expectedData, mtr.data)
}
}
Loading