cmd/kubernetes-static: fixes and improvements #148

invidian · 2021-07-09T08:00:23Z

No description provided.

src/definition/fetch_test.go

src/kubelet/metric/metric.go

src/metric/populate_test.go

src/prometheus/definition.go

cmd/kubernetes-static/main.go

cmd/kubernetes-static/readme.md

src/ksm/group.go

src/kubelet/metric/metric.go

gsanchezgavier

LGTM thanks!

cmd/kubernetes-static/main.go

gsanchezgavier · 2021-07-16T11:09:41Z

src/kubelet/metric/metric.go

@@ -192,7 +191,11 @@ func fetchVolumeStats(v v1.VolumeStats) (definition.RawMetrics, error) {
 }

 // GroupStatsSummary groups specific data for pods, containers and node
-func GroupStatsSummary(statsSummary v1.Summary) (definition.RawGroups, []error) {
+func GroupStatsSummary(statsSummary *v1.Summary) (definition.RawGroups, []error) {


this makes me think that statsSummary is going to be modified inside the function.

@ Rob Pike pls add const to go asap thanks

@ Rob Pike pls add const to go asap thanks

I subscribed to golang/go#22876 just yesterday 😄 I think Rust does a good job with mutable ref here.

this makes me think that statsSummary is going to be modified inside the function.

@gsanchezgavier I understand your concern. However, as far as I know, most of the time you should expect that function will not modify it's parameters, but rather return a mutated copy of the data. Modifying in place is a performance optimization IMO.

Given that this function returns a different format, I think it should be obvious to assume the summary won't be modified.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Right now the binary actually expect working directory to be root of the repository. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

To indicate that Go 1.16 should be used for building, so we can safely use go:embed directive for kubernetes-static. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Integration expects it at "api-server" while it was at "apiserver". Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

So when running shell do not spawn at the end of the output. That improves readability and usability, while it shouldn't interrupt any consumers, as usually trailing whitespace is properly handled. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Using 'gci' formatter. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

This way, binary is independent from host file system and it's only important during build. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

In majority of cases there is no need to use 'new' keyword. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

There is no need to use 'localhost' explicitly. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

So it's more intuitive to use 'go run'. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Part of #149 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Without this patch, following error is printed: "entity name and type are required when defining one" Part of #149 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

So they are less likely to get out of sync with other parts of the code. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Those metric may not always be available due to various reasons, marking them as optional silents them from reporting as errors. If I recall correctly: - createdAt, createdKind, createdBy, deploymentName metrics won't be available for e.g. static pods. - reason, message metrics will only be available for failing pods. - cpuRequestedCores, cpuLimitCores, memoryRequestedBytes, memoryLimitBytes will only be calculated for pods with resource requests configured. - pvc* metrics will only be available for volumes backed by actual PVC, not for EmptyDir volume etc. Refs #149 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

To at least align with version of latest test data we have. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

To use standard formatting and some minimal error annotation to make error tracing easier while debugging. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Loop labels are not really needed, as they always stop the closest loop anyway. Check for nil Containers slice is also not needed, since iterating over nil slice will result in no iterations anyway. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

So it's clear which is label name and metric name. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> Co-authored-by: Roberto Santalla <roobre@roobre.es>

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

It will now return a pointer to a summary, which should save some memory copying and is more standard approach for functions, which may return error in Go. Additionally, to make use of using structure pointer, GroupStatsSummary() method is also adopted to take a pointer of the summary. There are also improved error messages in GetMetricsData(), which should be more helpful while debugging. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

roobre

LGTM, thanks and killer work @invidian !

These errors are no longer returned and they were rebased-in by mistake.

invidian force-pushed the invidian/improvements branch from bb51770 to 6968c22 Compare July 9, 2021 08:32

invidian requested review from paologallinaharbur and roobre July 12, 2021 16:23

invidian mentioned this pull request Jul 13, 2021

Add node status conditions metrics #155

Merged

2 tasks

roobre reviewed Jul 14, 2021

View reviewed changes

invidian force-pushed the invidian/improvements branch 2 times, most recently from 95269ca to 21d1242 Compare July 14, 2021 17:35

invidian requested a review from roobre July 14, 2021 17:41

roobre reviewed Jul 15, 2021

View reviewed changes

invidian force-pushed the invidian/improvements branch from 21d1242 to 03c208f Compare July 16, 2021 06:16

invidian requested a review from roobre July 16, 2021 06:16

gsanchezgavier approved these changes Jul 16, 2021

View reviewed changes

invidian added 18 commits July 16, 2021 17:55

cmd/kubernetes-static/readme.md: remove trailing whitespace

5ff0519

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/readme.md: fix documentation

ce91b54

Right now the binary actually expect working directory to be root of the repository. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

go.mod: bump Go version to 1.16

31fd1df

To indicate that Go 1.16 should be used for building, so we can safely use go:embed directive for kubernetes-static. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/data: fix location of api-server metrics

4f00c39

Integration expects it at "api-server" while it was at "apiserver". Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/metric: automatically improve formatting

2e4e7b7

Using 'gci' formatter. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static: use go:embed for serving static data

55cfd0a

This way, binary is independent from host file system and it's only important during build. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: simplify service list initialization

5d61643

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: simplify mock client initialization

46d68da

In majority of cases there is no need to use 'new' keyword. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: simplify returned endpoint address

560845f

There is no need to use 'localhost' explicitly. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: remove unnecessary Sleep

442aa2c

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: improve imports naming

b4fe304

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static: use only single source file

e17588a

So it's more intuitive to use 'go run'. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: add missing Service objects

80c445c

Part of #149 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: fix collecting API server metrics

7640eeb

Without this patch, following error is printed: "entity name and type are required when defining one" Part of #149 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: use constants for controlplane components

1af8c98

So they are less likely to get out of sync with other parts of the code. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/definition: improve variable names a bit

1043873

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

invidian and others added 11 commits July 16, 2021 17:55

src/definition/fetch.go: improve error messages a bit

496c5dc

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

cmd/kubernetes-static/main.go: mock newer Kubernetes version

69d3765

To at least align with version of latest test data we have. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/definition/populate.go: improve error message formatting

ed5031d

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/ksm/group.go: improve errors a bit

0c98004

To use standard formatting and some minimal error annotation to make error tracing easier while debugging. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/ksm/group.go: small styling improvements

ab35179

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/kubelet: small styling improvements

5e9d085

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/prometheus: small styling improvements

f34fb77

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/prometheus: improve error messages

9329da1

So it's clear which is label name and metric name. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> Co-authored-by: Roberto Santalla <roobre@roobre.es>

src/metric/definition.go: improve error messages in toUtilization()

1060f61

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

src/definition: small styling and formatting improvements

5aa03b2

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>

invidian force-pushed the invidian/improvements branch from 03c208f to 124dabe Compare July 16, 2021 15:56

invidian force-pushed the invidian/improvements branch from 124dabe to 6885e7a Compare July 16, 2021 16:04

roobre approved these changes Jul 16, 2021

View reviewed changes

roobre merged commit 5f2ad02 into main Jul 16, 2021

roobre deleted the invidian/improvements branch July 16, 2021 16:17

roobre added a commit that referenced this pull request Jul 16, 2021

metric/test: re-remove errors from test after rebasing #148

96c90e0

These errors are no longer returned and they were rebased-in by mistake.

roobre mentioned this pull request Jul 16, 2021

Migrate to SDK v3 #160

Merged

roobre added a commit that referenced this pull request Jul 19, 2021

metric/test: re-remove errors from test after rebasing #148

bdd36fc

These errors are no longer returned and they were rebased-in by mistake.

roobre added a commit that referenced this pull request Jul 20, 2021

metric/test: re-remove errors from test after rebasing #148

ad28605

These errors are no longer returned and they were rebased-in by mistake.

roobre added a commit that referenced this pull request Jul 20, 2021

metric/test: re-remove errors from test after rebasing #148

eedbdb6

These errors are no longer returned and they were rebased-in by mistake.

roobre added a commit that referenced this pull request Jul 23, 2021

metric/test: re-remove errors from test after rebasing #148

bb1291c

These errors are no longer returned and they were rebased-in by mistake.

roobre added a commit that referenced this pull request Jul 27, 2021

metric/test: re-remove errors from test after rebasing #148

76db41f

These errors are no longer returned and they were rebased-in by mistake.

roobre added a commit that referenced this pull request Jul 28, 2021

metric/test: re-remove errors from test after rebasing #148

bcf6ed9

These errors are no longer returned and they were rebased-in by mistake.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/kubernetes-static: fixes and improvements #148

cmd/kubernetes-static: fixes and improvements #148

invidian commented Jul 9, 2021

gsanchezgavier left a comment

gsanchezgavier Jul 16, 2021

roobre Jul 16, 2021

invidian Jul 16, 2021

roobre left a comment

cmd/kubernetes-static: fixes and improvements #148

cmd/kubernetes-static: fixes and improvements #148

Conversation

invidian commented Jul 9, 2021

gsanchezgavier left a comment

Choose a reason for hiding this comment

gsanchezgavier Jul 16, 2021

Choose a reason for hiding this comment

roobre Jul 16, 2021

Choose a reason for hiding this comment

invidian Jul 16, 2021

Choose a reason for hiding this comment

roobre left a comment

Choose a reason for hiding this comment