Modify clickhouse statistic related func for theia-manager #132

yuntanghsu · 2022-10-24T18:04:19Z

This PR modifies and adds the clickhouse statistic related functions for Theia Manager:

Define REST request and return payload struct for related API endpoints and add code generation under pkg/apis/stats
a. REST endpoint apis/stats.theia.antrea.io/v1alpha1/clickhouse
Adapt CLI to use Theia Manager
Modify e2e test

The changes are split into two chunks (to hopefully help make reviews easier), with one commit focusing on adding status API, another one implementing the CLI changes and modifying the e2e test,

Signed-off-by: Yun-Tang Hsu [email protected]

yuntanghsu · 2022-10-24T18:11:50Z

/theia-test-e2e

yuntanghsu · 2022-10-25T21:00:52Z

/theia-test-e2e

wsquan171

overall lgtm. some nits.

pkg/util/utils.go

pkg/controller/stats/controller.go

pkg/apis/stats/v1alpha1/types.go

wsquan171

LGTM. one simple nit.

pkg/apis/stats/v1alpha1/types.go

pkg/controller/stats/clickhouse_stats.go

pkg/util/utils.go

yanjunz97 · 2022-11-21T20:20:05Z

pkg/util/utils.go

@@ -0,0 +1,115 @@
+// Copyright 2022 Antrea Authors.


I also tried to move some utility functions to the common pkg in PR #138 . I think most of the functions are similar and the later merged PR can fix the conflicts, but I would like to get some feedbacks about if we prefer to put all these functions in one file like this one, or classify them into multiple pkgs like clickhouse, policyrecommendation, k8s as I do in PR #138

I don't have strong opinion regarding this, but I might prefer classifying them into multiple pkgs.

pkg/apis/stats/v1alpha1/types.go

Signed-off-by: Yun-Tang Hsu <[email protected]>

yuntanghsu · 2022-11-29T05:53:56Z

/theia-test-e2e

build/charts/theia/templates/theia-manager/service.yaml

pkg/apis/stats/v1alpha1/types.go

salv-orlando

Overall LGTM, just some nits inline

salv-orlando · 2022-11-30T16:01:03Z

ci/jenkins/test-vmc.sh

@@ -339,7 +339,7 @@ function deliver_antrea {

    ${GIT_CHECKOUT_DIR}/hack/generate-manifest.sh --ch-size 100Mi --ch-monitor-threshold 0.1 > ${GIT_CHECKOUT_DIR}/build/yamls/flow-visibility.yml
    ${GIT_CHECKOUT_DIR}/hack/generate-manifest.sh --no-grafana --spark-operator --theia-manager > ${GIT_CHECKOUT_DIR}/build/yamls/flow-visibility-with-spark.yml
-    ${GIT_CHECKOUT_DIR}/hack/generate-manifest.sh --no-grafana > ${GIT_CHECKOUT_DIR}/build/yamls/flow-visibility-ch-only.yml
+    ${GIT_CHECKOUT_DIR}/hack/generate-manifest.sh --no-grafana --theia-manager > ${GIT_CHECKOUT_DIR}/build/yamls/flow-visibility-ch-only.yml


were we lacking theia manager in the manifests we use for testing before this change?

We previously added theia manager at line #341 for NPR related functions in CLI. Line #342 is for the clickhouse related changes in this PR.

pkg/apiserver/utils/stats/clickhouse_stats.go

salv-orlando · 2022-11-30T16:14:19Z

pkg/apiserver/utils/stats/clickhouse_stats.go

+			stats.StackTraces = append(stats.StackTraces, res)
+		}
+		if err != nil {
+			return fmt.Errorf("failed to parse the data returned by database: %v", err)


This is just to make a little less ignorant... can this query return multiple rows?

In that case, would it better to skip and move to the next record if we fail to parse a record?

The query returns multiple rows.
I think it is a good idea to skip the error row instead of throwing away the whole data?

cc @yanjunz97 and @wsquan171 for some feedbacks.

Agree to skip the error row. It would be better if we could display the results retrieved successfully and the error regarding to the error row at the same time?

I added a ErrorMsg, which is a []string, in ClickHouseStats. If we have an error row, we will skip it and add the error msg to ErrorMsg. On CLI side, it will show both ErrorMsgs and the results.

The new approach looks good to me.

Signed-off-by: Yun-Tang Hsu <[email protected]>

yanjunz97

LGTM

yuntanghsu · 2022-12-01T03:02:23Z

/theia-test-e2e

yuntanghsu marked this pull request as draft October 24, 2022 18:04

yuntanghsu force-pushed the clickhouse_status branch 4 times, most recently from 4017672 to 07468bc Compare October 25, 2022 20:31

yuntanghsu marked this pull request as ready for review October 26, 2022 18:05

yuntanghsu force-pushed the clickhouse_status branch from 07468bc to 1160f1d Compare October 26, 2022 23:52

yuntanghsu added this to the Theia v0.4 release milestone Oct 26, 2022

yuntanghsu requested review from yanjunz97 and wsquan171 October 27, 2022 00:44

wsquan171 reviewed Nov 17, 2022

View reviewed changes

pkg/util/utils.go Outdated Show resolved Hide resolved

pkg/controller/stats/controller.go Outdated Show resolved Hide resolved

pkg/controller/stats/controller.go Outdated Show resolved Hide resolved

pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_status branch 3 times, most recently from 5df9906 to a1c7d59 Compare November 17, 2022 22:15

wsquan171 approved these changes Nov 18, 2022

View reviewed changes

pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_status branch from a1c7d59 to 74a2b2a Compare November 18, 2022 01:43

yanjunz97 reviewed Nov 21, 2022

View reviewed changes

yuntanghsu force-pushed the clickhouse_status branch from 74a2b2a to 5563fc4 Compare November 28, 2022 22:17

yuntanghsu added 2 commits November 28, 2022 14:17

Add stats/clickhouse rest handler

81113fb

Signed-off-by: Yun-Tang Hsu <[email protected]>

Theia CLI and e2e test modification

f4fc3d6

Signed-off-by: Yun-Tang Hsu <[email protected]>

yuntanghsu force-pushed the clickhouse_status branch 2 times, most recently from 8f9232e to ea35d5c Compare November 28, 2022 22:35

Address comments

c7492db

Signed-off-by: Yun-Tang Hsu <[email protected]>

yuntanghsu force-pushed the clickhouse_status branch 4 times, most recently from 4769b15 to cded8f6 Compare November 29, 2022 03:50

yanjunz97 reviewed Nov 29, 2022

View reviewed changes

build/charts/theia/templates/theia-manager/service.yaml Outdated Show resolved Hide resolved

pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_status branch from cded8f6 to e63a611 Compare November 30, 2022 01:42

salv-orlando reviewed Nov 30, 2022

View reviewed changes

restructure ClickHouseStats

bdaa510

Signed-off-by: Yun-Tang Hsu <[email protected]>

yuntanghsu force-pushed the clickhouse_status branch from e63a611 to bdaa510 Compare November 30, 2022 23:07

yanjunz97 approved these changes Dec 1, 2022

View reviewed changes

yuntanghsu merged commit b6c1c1f into antrea-io:main Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify clickhouse statistic related func for theia-manager #132

Modify clickhouse statistic related func for theia-manager #132

yuntanghsu commented Oct 24, 2022 •

edited

Loading

yuntanghsu commented Oct 24, 2022

yuntanghsu commented Oct 25, 2022

wsquan171 left a comment

wsquan171 left a comment

yanjunz97 Nov 21, 2022 •

edited

Loading

yuntanghsu Nov 29, 2022

yuntanghsu commented Nov 29, 2022

salv-orlando left a comment

salv-orlando Nov 30, 2022

yuntanghsu Nov 30, 2022

salv-orlando Nov 30, 2022

yuntanghsu Nov 30, 2022 •

edited

Loading

yanjunz97 Nov 30, 2022

yuntanghsu Nov 30, 2022

wsquan171 Dec 1, 2022

yanjunz97 left a comment

yuntanghsu commented Dec 1, 2022

Modify clickhouse statistic related func for theia-manager #132

Modify clickhouse statistic related func for theia-manager #132

Conversation

yuntanghsu commented Oct 24, 2022 • edited Loading

yuntanghsu commented Oct 24, 2022

yuntanghsu commented Oct 25, 2022

wsquan171 left a comment

Choose a reason for hiding this comment

wsquan171 left a comment

Choose a reason for hiding this comment

yanjunz97 Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

yuntanghsu Nov 29, 2022

Choose a reason for hiding this comment

yuntanghsu commented Nov 29, 2022

salv-orlando left a comment

Choose a reason for hiding this comment

salv-orlando Nov 30, 2022

Choose a reason for hiding this comment

yuntanghsu Nov 30, 2022

Choose a reason for hiding this comment

salv-orlando Nov 30, 2022

Choose a reason for hiding this comment

yuntanghsu Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

yanjunz97 Nov 30, 2022

Choose a reason for hiding this comment

yuntanghsu Nov 30, 2022

Choose a reason for hiding this comment

wsquan171 Dec 1, 2022

Choose a reason for hiding this comment

yanjunz97 left a comment

Choose a reason for hiding this comment

yuntanghsu commented Dec 1, 2022

yuntanghsu commented Oct 24, 2022 •

edited

Loading

yanjunz97 Nov 21, 2022 •

edited

Loading

yuntanghsu Nov 30, 2022 •

edited

Loading