add internal monitoring server for metrics and profiling #3500

enekofb · 2023-10-16T14:44:00Z

What changed?

Do not exposed profiling endpoint in the main server
Created monitoring server with metrics and profiling endpoint
Monitoring remains private
refactored unit tests to use mocked httpserver for less flaky tests

Why was this change made?

We wanted to be able to release profiling feature but we could not cause it was in the same public endpoint as the API.

How was this change implemented?

Refactored the existing metrics server to be more general for monitoring purposes. Initially called management server but renamed to monitoring server as the term is already overloaded in our context.
Refactored the configuration around this also.

How did you validate the change?

Explain how a reviewer can verify the change themselves

Tilt:

run the app with default values that it has monitoring and metrics server by default. If you want to enable profiling, do it in your values.
then try to consume the /debug/pprof endpoint from the main server, you should see an error (meaning that is not publicly exposed)

- then go and expose the monitoring server (running in port 8080 by default) and consume

/metrics endpoint

/debug/pprof endpoint

Integration tests -- what is covered, what cannot be covered;
or, explain why there are no new tests

this test is border-line between integration and unit https://github.com/weaveworks/weave-gitops-
enterprise/pull/3500/files#diff-c426c36708aa75ac5531273cbf747c1cdda4953cee6346db63de5e392e6fb87c

Unit tests -- what is covered, what cannot be covered; are
there tests that fail without the change?

Added and refactored some existing ones

Release notes

⚠️ Breaking Change

We have updated configuration values embedding metrics under monitoring. In case you
have disabled metrics in your helm release values previously by setting:

metrics:
  enabled: false

You should update it to have he same behavior

monitoring:
  enabled: false

Please review monitoring for more info.

Documentation Changes

weaveworks/weave-gitops#4089

Other follow ups

Not identified

cmd/clusters-service/app/server.go

pkg/monitoring/profiling/pprof_test.go

enekofb · 2023-10-17T15:35:57Z

@squaremo @bigkevmcd just adding your for visibility on the change. Feel free to review on marked as ready if you want but not needed.

cmd/clusters-service/app/server.go

bigkevmcd · 2023-10-17T16:34:58Z

pkg/monitoring/profiling/pprof.go

+	Enabled bool
+}
+
+func NewDefaultPprofHandler() (string, http.Handler) {


Exported, so should have a doc comment?

pkg/monitoring/server.go

bigkevmcd · 2023-10-17T16:36:02Z

pkg/monitoring/server.go

+
+// NewSever creates and starts a management server for all endpoints that we need to expose internally. For example metrics or profiling.
+func NewServer(opts Options) (*http.Server, error) {
+	if !opts.Enabled {


It feels like this should be moved outside, so we only create the server if it's enabled?

agree, it was odd. the proper check was put in the client side. removed here

bigkevmcd · 2023-10-17T16:36:49Z

cmd/clusters-service/app/server.go

+	// management server
+	var monitoringServer *http.Server
+	args.Log.Info("i am in server monitoring", "monitoring", args.MonitoringOptions)
+	if args.MonitoringOptions.Enabled {


See below, maybe we shouldn't be creating the server if it's not enabled?

i think this is covered by the previous comment. let me know in case not.

pkg/monitoring/server.go

bigkevmcd · 2023-10-17T16:48:32Z

pkg/monitoring/server_test.go

+)
+
+func TestNewServer(t *testing.T) {
+	require := require.New(t)


I don't think this is right, you're calling .Error inside a sub-test which will trigger a subtest error?

Thanks for the comment: could you expand a bit more. My idea here is to have a test case for unhappy path, in this case where a client tries to create a monitoring server without a valid address.

Looks like it's fixed now?

Essentially, failing a *testing.T from a parent test will cause the subtest to fail with an error.

package main import ( "testing" "github.com/stretchr/testify/require" ) func TestHelloWorld(t *testing.T) { require := require.New(t) value := "testing errors" t.Run("bad subtest", func(t *testing.T) { require.Equal(value, "test") }) t.Run("this subtest", func(t *testing.T) { require.Equal(value, "another test") }) }

$ go test . --- FAIL: TestHelloWorld (0.00s) subtest_test.go:14: Error Trace: /Users/kevin/Source/Go/eneko/subtest_test.go:14 Error: Not equal: expected: "testing errors" actual : "test" Diff: --- Expected +++ Actual @@ -1 +1 @@ -testing errors +test Test: TestHelloWorld --- FAIL: TestHelloWorld/bad_subtest (0.00s) testing.go:1490: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test FAIL FAIL subtest-demo 0.176s FAIL

ohh yes, I see got it now ... that require was not supposed to be there. thanks for the explanation 🙏

bigkevmcd · 2023-10-17T16:49:45Z

charts/mccp/templates/clusters-service/deployment.yaml

-            - name: http-metrics
-              containerPort: {{ .Values.metrics.service.port }}
+          {{- if .Values.monitoring.enabled }}
+            - name: {{ .Values.monitoring.service.name }}


bigkevmcd · 2023-10-17T16:49:51Z

charts/mccp/templates/clusters-service/service.yaml

-    {{- if .Values.metrics.enabled }}
-    - port: {{ .Values.metrics.service.port }}
-      targetPort: {{ .Values.metrics.service.port }}
+    {{- if .Values.monitoring.enabled }}


cmd/clusters-service/app/options.go

pkg/monitoring/metrics/prometheus_test.go

pkg/monitoring/profiling/pprof_test.go

enekofb · 2023-10-18T11:03:45Z

Added also docs to the PR and here weaveworks/weave-gitops#4089

enekofb · 2023-10-19T07:36:51Z

@bigkevmcd please let me know whether request for changes still applies or I could go ahead merging?

enekofb commented Oct 16, 2023

View reviewed changes

cmd/clusters-service/app/server.go Outdated Show resolved Hide resolved

enekofb force-pushed the issues/3228 branch from 27ddf49 to c5c5866 Compare October 17, 2023 14:37

enekofb requested review from squaremo, bigkevmcd and a team October 17, 2023 14:49

enekofb commented Oct 17, 2023

View reviewed changes

pkg/monitoring/profiling/pprof_test.go Outdated Show resolved Hide resolved

enekofb added the enhancement New feature or request label Oct 17, 2023

enekofb changed the title ~~do not expose profiling debug endpoint~~ add internal monitoring server for metrics and profiling telemetry Oct 17, 2023

enekofb changed the title ~~add internal monitoring server for metrics and profiling telemetry~~ add internal monitoring server for metrics and profiling Oct 17, 2023

enekofb marked this pull request as ready for review October 17, 2023 16:31

enekofb force-pushed the issues/3228 branch from 1761ce8 to cd2ac1f Compare October 17, 2023 16:36

bigkevmcd requested changes Oct 17, 2023

View reviewed changes

jpellizzari reviewed Oct 17, 2023

View reviewed changes

pkg/monitoring/metrics/prometheus_test.go Outdated Show resolved Hide resolved

pkg/monitoring/profiling/pprof_test.go Outdated Show resolved Hide resolved

enekofb requested review from bigkevmcd and jpellizzari October 18, 2023 11:03

enekofb added the breaking-change to flag breaking changes similar to weave gitops oss label Oct 18, 2023

jpellizzari approved these changes Oct 18, 2023

View reviewed changes

enekofb added 11 commits October 19, 2023 08:35

added test

18e83b3

created management server

f28bdd3

adding management and profiling

730b6b9

added configuration

817d1f8

refactoring to monitoring

a476373

refactored configuration

9b45787

can create monitoring server

b7439ba

created monitoring server with metrics and profiling endpoints

dc47bf5

unit test refactored after monitoring server joined

c0a5d43

missed changes

6f6c078

removed debugging msgs

f7def67

enekofb added 6 commits October 19, 2023 08:35

review suggestions: added comments

b0a6a8e

removed enabled checking on creating server

832807a

refactored tests

414c79a

made more clear that is the site

2502f0c

enhanced assertions

fc44a67

documented options

5b1b959

enekofb force-pushed the issues/3228 branch from 479b941 to 5b1b959 Compare October 19, 2023 07:35

bigkevmcd approved these changes Oct 19, 2023

View reviewed changes

enekofb merged commit 9c5f9ec into main Oct 19, 2023
10 checks passed

enekofb deleted the issues/3228 branch October 19, 2023 07:55

This was referenced Oct 19, 2023

added port to reflect changes in wge monitoring server weaveworks/weave-gitops-quickstart#8

Merged

Add granular control of query service components #3485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add internal monitoring server for metrics and profiling #3500

add internal monitoring server for metrics and profiling #3500

enekofb commented Oct 16, 2023 •

edited

Loading

enekofb commented Oct 17, 2023

bigkevmcd Oct 17, 2023

enekofb Oct 18, 2023

bigkevmcd Oct 17, 2023

enekofb Oct 18, 2023 •

edited

Loading

bigkevmcd Oct 17, 2023

enekofb Oct 18, 2023

bigkevmcd Oct 17, 2023

enekofb Oct 18, 2023

bigkevmcd Oct 18, 2023

enekofb Oct 18, 2023 •

edited

Loading

bigkevmcd Oct 17, 2023

bigkevmcd Oct 17, 2023

enekofb commented Oct 18, 2023 •

edited

Loading

enekofb commented Oct 19, 2023 •

edited

Loading

add internal monitoring server for metrics and profiling #3500

add internal monitoring server for metrics and profiling #3500

Conversation

enekofb commented Oct 16, 2023 • edited Loading

enekofb commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enekofb Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enekofb Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enekofb commented Oct 18, 2023 • edited Loading

enekofb commented Oct 19, 2023 • edited Loading

enekofb commented Oct 16, 2023 •

edited

Loading

enekofb Oct 18, 2023 •

edited

Loading

enekofb Oct 18, 2023 •

edited

Loading

enekofb commented Oct 18, 2023 •

edited

Loading

enekofb commented Oct 19, 2023 •

edited

Loading