Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
13 changes: 12 additions & 1 deletion internal/controller/telemetry/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"os"
"runtime"
"sort"
"strings"
Expand Down Expand Up @@ -40,7 +41,7 @@ type ConfigurationGetter interface {
// Data is telemetry data.
//
//go:generate go run -tags generator github.com/nginx/telemetry-exporter/cmd/generator -type=Data -scheme -scheme-protocol=NGFProductTelemetry -scheme-df-datatype=ngf-product-telemetry
type Data struct {
type Data struct { //nolint //required to skip golangci-lint-full fieldalignment
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need nolint here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The golangci linter sets a limit on the number of characters allowed in a struct. I can't remember what the exact error was, but it was related to fieldalignment. If I remember correctly the linter errored as we could have had less pointer bytes overall (not 100% sure what that means)

It resolved it by remving the comments around the fields in the struct. The issue with that those comments are required as part of the go generate command when we are generating the data.avdl file for telemetry

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not quite right. The fieldalignment linter helps to optimize Go structs so that they take up less memory in the binary at compilation. The linter automatically fixes these issues by reorganizing the struct to fit the most optimal order, but there's a bug that causes the comments to be removed.

We don't want to nolint here. The solution is either let the linter reorganize, and then manually add the comments back in, or in this case you can probably satisfy the linter by manually putting the BuildOS string alongside the other strings in the struct, because having the same types next to each other helps with the optimization. But we should not ignore these with a nolint.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sjberman the issue is that the data.avdl file is auto-generated by this Data struct from the telemetry-exporter. And we can't change the order of the data.avdl elements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, thanks for the clarification.

// ImageSource tells whether the image was built by GitHub or locally (values are 'gha', 'local', or 'unknown')
ImageSource string
tel.Data // embedding is required by the generator.
Expand All @@ -66,6 +67,8 @@ type Data struct {
ControlPlanePodCount int64
// NginxOneConnectionEnabled is a boolean that indicates whether the connection to the Nginx One Console is enabled.
NginxOneConnectionEnabled bool
// BuildOS is the base operating system the control plane was built on (e.g. alpine, ubi).
BuildOS string
}

// NGFResourceCounts stores the counts of all relevant resources that NGF processes and generates configuration from.
Expand Down Expand Up @@ -121,6 +124,8 @@ type DataCollectorConfig struct {
Version string
// ImageSource is the source of the NGF image.
ImageSource string
// BuildOS is the base operating system the control plane was built on (e.g. alpine, ubi).
BuildOS string
// Flags contains the command-line NGF flag keys and values.
Flags config.Flags
// NginxOneConsoleConnection is a boolean that indicates whether the connection to the Nginx One Console is enabled.
Expand Down Expand Up @@ -174,6 +179,11 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) {

nginxPodCount := getNginxPodCount(g, clusterInfo.NodeCount)

buildOs := os.Getenv("BUILD_OS")
if buildOs == "" {
buildOs = "alpine"
}

data := Data{
Data: tel.Data{
ProjectName: "NGF",
Expand All @@ -187,6 +197,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) {
},
NGFResourceCounts: graphResourceCount,
ImageSource: c.cfg.ImageSource,
BuildOS: buildOs,
FlagNames: c.cfg.Flags.Names,
FlagValues: c.cfg.Flags.Values,
SnippetsFiltersDirectives: snippetsFiltersDirectives,
Expand Down
3 changes: 3 additions & 0 deletions internal/controller/telemetry/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ var _ = Describe("Collector", Ordered, func() {
NGFResourceCounts: telemetry.NGFResourceCounts{},
ControlPlanePodCount: 1,
ImageSource: "local",
BuildOS: "alpine",
FlagNames: flags.Names,
FlagValues: flags.Values,
SnippetsFiltersDirectives: []string{},
Expand All @@ -193,6 +194,7 @@ var _ = Describe("Collector", Ordered, func() {
Version: version,
PodNSName: podNSName,
ImageSource: "local",
BuildOS: "alpine",
Flags: flags,
NginxOneConsoleConnection: true,
})
Expand Down Expand Up @@ -519,6 +521,7 @@ var _ = Describe("Collector", Ordered, func() {
expData.NginxPodCount = int64(8)
expData.ControlPlanePodCount = int64(2)
expData.NginxOneConnectionEnabled = true
expData.BuildOS = "alpine"

data, err := dataCollector.Collect(ctx)
Expect(err).ToNot(HaveOccurred())
Expand Down
3 changes: 3 additions & 0 deletions internal/controller/telemetry/data.avdl
Original file line number Diff line number Diff line change
Expand Up @@ -114,5 +114,8 @@ attached at the Gateway level. */
/** NginxOneConnectionEnabled is a boolean that indicates whether the connection to the Nginx One Console is enabled. */
boolean? NginxOneConnectionEnabled = null;

/** BuildOS is the base operating system the control plane was built on (e.g. alpine, ubi). */
string? BuildOS = null;

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ func (d *Data) Attributes() []attribute.KeyValue {
attrs = append(attrs, attribute.Int64("NginxPodCount", d.NginxPodCount))
attrs = append(attrs, attribute.Int64("ControlPlanePodCount", d.ControlPlanePodCount))
attrs = append(attrs, attribute.Bool("NginxOneConnectionEnabled", d.NginxOneConnectionEnabled))
attrs = append(attrs, attribute.String("BuildOS", d.BuildOS))

return attrs
}
Expand Down
2 changes: 2 additions & 0 deletions internal/controller/telemetry/data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ func TestDataAttributes(t *testing.T) {
attribute.Int64("NginxPodCount", 3),
attribute.Int64("ControlPlanePodCount", 3),
attribute.Bool("NginxOneConnectionEnabled", true),
attribute.String("BuildOS", ""),
}

result := data.Attributes()
Expand Down Expand Up @@ -132,6 +133,7 @@ func TestDataAttributesWithEmptyData(t *testing.T) {
attribute.Int64("NginxPodCount", 0),
attribute.Int64("ControlPlanePodCount", 0),
attribute.Bool("NginxOneConnectionEnabled", false),
attribute.String("BuildOS", ""),
}

result := data.Attributes()
Expand Down
1 change: 1 addition & 0 deletions tests/suite/telemetry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func(
"NginxPodCount: Int(0)",
"ControlPlanePodCount: Int(1)",
"NginxOneConnectionEnabled: Bool(false)",
"BuildOS:",
},
)
})
Expand Down
Loading