add otel metrics for spiderpool #81

Icarus9913 · 2022-03-29T15:17:55Z

Signed-off-by: Icarus9913 icaruswu66@qq.com

Add otel metrics module for spiderpool to monitor.

Test

pkg/metrics/metrics_test.go

+		metricController.RegisterMetric("Node_IP_Allocation_Counts", "The total counts of node IP allocations", metrics.COUNTER)
+		metricController.RegisterMetric("Node_IP_Allocation_Duration", "The duration of node IP allocations", metrics.HISTOGRAM)
+		metricController.SetMetricAlert("Node_IP_Allocation_Counts", int64(1), int64(1000))
+		metricController.SetMetricValue("Node_IP_Allocation_Counts", int64(3), map[string]string{"type": "total", "node": "node1"})


Icarus9913 · 2022-03-30T06:45:08Z

pkg/metrics/metrics.go

+// TODO: Should we return the data encoded in JSON?
+// GetMetricDatabase returns the current metricController metricInstance.
+func (mc *metricController) GetMetricDatabase() map[string]*Metric {
+	return mc.metricInstance


此处返回database是否需要执行json.marshal，最后返回一个string字符串？
因为直接打印这个map[string]*Metric，这里有指针，而打印并不会去解析指针的值，而是直接输出地址。

pkg/metrics/metrics.go

+}
+
+// NewMetricController will create a MetricController instance.
+func NewMetricController(meterName string) (MetricController, func(http.ResponseWriter, *http.Request), error) {


weizhoublue · 2022-03-30T07:28:39Z

pkg/metrics/metrics.go

+	case HISTOGRAM:
+		m.Metric = metric.Must(mc.meter).NewFloat64Histogram(metricName, metric.WithDescription(description))
+	}
+	mc.metricInstance[metricName] = m


what about a metricName already exists ?

pkg/metrics/metrics.go

+}
+
+// RegisterMetric registers metrics.
+func (mc *metricController) RegisterMetric(metricName, description string, metricType MetricType) {


pkg/metrics/metrics.go

+		mi.AlertLimitUp = limitUp
+		mi.AlertLimitDown = limitDown
+	} else {
+		fmt.Printf("%s does not exist", metricName)


pkg/metrics/metrics.go

+func (mc *metricController) SetMetricAlert(metricName string, limitUp, limitDown interface{}) {
+	if mi, ok := mc.metricInstance[metricName]; ok {
+		mi.AlertLimitUp = limitUp
+		mi.AlertLimitDown = limitDown


pkg/metrics/metrics.go

+}
+
+// RecordTimeMetric returns a function which will record the duration after being called.
+func (mc *metricController) RecordTimeMetric(metricName string, attrList map[string]string) (fn func()) {


pkg/metrics/metrics.go

+
+	switch metricType {
+	case COUNTER:
+		m.Metric = metric.Must(mc.meter).NewInt64Counter(metricName, metric.WithDescription(description))


pkg/metrics/metrics.go

+		return
+	}
+
+	fmt.Printf("%s does not exist", metricName)


pkg/metrics/metrics.go

+	if m, ok := mc.metricInstance[metricName]; ok {
+		if m.Mtype != HISTOGRAM {
+			fmt.Printf("%s is not %v type to record duration!", metricName, HISTOGRAM)
+			return


pkg/metrics/metrics_test.go

+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	It("use prometheus as exporter", func() {


weizhoublue · 2022-03-30T08:05:42Z

每个函数的健壮性不好，没有入参校验，错误了没有抛给外部
单元测试太简单，需要有正向和反向用例，甚至是并发竞争，且覆盖率100%

Icarus9913 · 2022-03-30T08:20:24Z

metric 部分的设计，是否应该做成不对业务影响的模块，不耦合？对此，我只在新建接口时才有过一次err异常抛出。
指标收集部分的代码是插入在业务代码中，应该仅作为一个插入，而不应该对业务层有err介入，否则写业务会因为没有记录到指标而抛异常？我的想法是，中间键部分的代码，仅仅作为业务代码部门的某一两行的污染，若有异常应log指出。不应让其err影响到业务层？

weizhoublue · 2022-03-30T08:21:32Z

pkg/metrics/metrics.go

+	processor "go.opentelemetry.io/otel/sdk/metric/processor/basic"
+	selector "go.opentelemetry.io/otel/sdk/metric/selector/simple"
+)
+


it could be a default logger to use
var loger = log.Stdloger.(" metric")

weizhoublue · 2022-03-30T08:36:37Z

pkg/metrics/metrics_test.go

+		metricController, httpHandle, err = metrics.NewMetricController(SpiderPoolMeter)
+		Expect(err).NotTo(HaveOccurred())
+	})
+


it is easy to use DescribeTable to cover more test case

weizhoublue · 2022-03-30T14:18:31Z

不让外部感知 erro 不叫 “不耦合”，外部不要 call function call 才叫不耦合
没见过哪个开源的 module 能自己内部消化掉 error，整个 function call 都失败了，还完全不让外部感知
应该要传递 error 出来，上层业务是否忽略或处理，是上层的事，打印出错也可以上层自己的logger 来控制。
不需要本pkg 操心 error 的处理，否则外部业务完全丧失控制

pkg/metrics/metrics.go

+	if len(metricName) == 0 {
+		return metric.Int64Counter{}, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.")
+	}
+	return metric.Must(meter).NewInt64Counter(metricName, metric.WithDescription(description)), nil


weizhoublue · 2022-04-09T03:06:01Z

pkg/metrics/metrics.go

+	if len(metricName) == 0 {
+		return metric.Float64Histogram{}, fmt.Errorf("Failed to create metric Float64Histogram, metric name is asked to be set.")
+	}
+	return metric.Must(meter).NewFloat64Histogram(metricName, metric.WithDescription(description)), nil


if description is empty, metric.WithDescription() will go wrong ?

pkg/metrics/metrics.go

+	if len(metricName) == 0 {
+		return metric.Float64Histogram{}, fmt.Errorf("Failed to create metric Float64Histogram, metric name is asked to be set.")
+	}
+	return metric.Must(meter).NewFloat64Histogram(metricName, metric.WithDescription(description)), nil


Icarus9913 · 2022-04-11T10:00:08Z

pkg/metrics/metrics.go

+	if len(metricName) == 0 {
+		return nil, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.")
+	}
+	return meter.SyncInt64().Counter(metricName, instrument.WithDescription(description))


已规避并发问题。syncInt64采用CAS自旋方式来解决并发问题。

https://github.com/open-telemetry/opentelemetry-go/blob/18f4cb85ece82b12cb9bd9af02efe2a47bd8f76e/sdk/metric/number/number.go#L386

pkg/metrics/metrics.go

+	if len(metricName) == 0 {
+		return metric.Int64Counter{}, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.")
+	}
+	return metric.Must(meter).NewInt64Counter(metricName, metric.WithDescription(description)), nil


pkg/metrics/metrics_test.go

+
+		close(c)
+		//Consistently(c, "60s").Should(BeClosed())
+		Eventually(c, "20s").Should(BeClosed())


weizhoublue · 2022-04-13T12:27:30Z

pkg/metrics/metrics_test.go

+		http.HandleFunc("/metrics", httpHandle)
+		go func() {
+			_ = http.ListenAndServe(":2222", nil)
+		}()


第一，这个 http server 启动后，在 it 中没有进行 client 消费，没有起到真正的作用，未能验证 metric真正的能输出正确的值
第二，，在整工程跑单元测试时，这个协程泄露，没有回收
DeferCleanup(func() {
close http server
})

建议 if server.ListenAndServe() { printf " I am existing , info=%v ". %v} ，万一本地端口本占用呢？或者中途挂了，或者知道 afterall 通知推出了

pkg/metrics/metrics_test.go

+	SpiderPoolMeter                    = "spider_pool_meter"
+	MetricNodeIPAllocationCountsName   = "Node_IP_Allocation_Counts"
+	MetricNodeIPAllocationDurationName = "Node_IP_Allocation_Duration"
+)


pkg/metrics/metrics_test.go

+	MetricNodeIPAllocationDuration syncfloat64.Histogram
+}
+
+var _ = Describe("metrics", Label("unitest"), Ordered, func() {


codecov · 2022-04-18T15:43:03Z

Codecov Report

Merging #81 (0a8b6d0) into main (1621ec6) will increase coverage by 3.94%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   77.58%   81.52%   +3.94%     
==========================================
  Files           2        3       +1     
  Lines         116      157      +41     
==========================================
+ Hits           90      128      +38     
- Misses         19       21       +2     
- Partials        7        8       +1

Flag	Coverage Δ
unittests	`81.52% <92.68%> (+3.94%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/metrics/metrics.go	`92.68% <92.68%> (ø)`

Icarus9913 · 2022-04-19T03:25:58Z

对于metrics的unitest记录某种指标是否成功记录/消费的验证，otel库目前缺少该功能。已有其他contributor提过该类的issue和pr，但是还没有merge。
open-telemetry/opentelemetry-go#2776

weizhoublue · 2022-04-21T03:22:57Z

有冲突了，解决下

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

weizhoublue · 2022-04-22T17:49:49Z

pkg/metrics/metrics_suite_test.go

+
+		err = server.Serve(listener)
+		if nil != err && err != http.ErrServerClosed && err != net.ErrClosed {
+			logger.Error(err.Error())


ginkgo 用的是 BY() 或 FAIL() 或 GinkgoWriter.Printf , 这种日志才能追加到测试报告中，
没有使用 logger 或者 printf 的，单元测试成千上万的日志，是没法看的，也不能在测试报告中看到

weizhoublue · 2022-04-22T18:17:46Z

pkg/metrics/metrics_test.go

+				}
+
+				// verify counts instrument
+				if strings.Contains(line, MetricNodeIPAllocationCountsName) && strings.Contains(line, OTEL_COUNTS_SIGNAL) {


如果是能把 json字符串解析成 struct，使用 struct 成员来值判断，会是更加正道的方法
尤其是一些 validator 库的使用，能加速效率
https://github.com/go-playground/validator/blob/master/_examples/simple/main.go
https://github.com/gookit/validate/blob/master/README.zh-CN.md

prometheus handler对于http.ResponseWriter，是采用text文本返回。

https://github.com/prometheus/common/blob/627089d3a7af73be778847aa577192b937b8d89a/expfmt/encode.go#L86
https://github.com/prometheus/common/blob/627089d3a7af73be778847aa577192b937b8d89a/expfmt/text_create.go#L67

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

# Conflicts: # go.mod # go.sum # vendor/modules.txt

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

sonarcloud · 2022-04-23T14:41:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2022-07-09T20:20:33Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

Icarus9913 added the pr/release/feature-new label Mar 29, 2022

Icarus9913 requested a review from weizhoublue as a code owner March 29, 2022 15:17

Icarus9913 force-pushed the feat/wk/otel-metrics branch from a022723 to 96467b1 Compare March 29, 2022 16:40

Icarus9913 commented Mar 30, 2022

View reviewed changes

weizhoublue reviewed Mar 30, 2022

View reviewed changes

weizhoublue added this to the kinds of pkg module milestone Mar 30, 2022

weizhoublue linked an issue Mar 30, 2022 that may be closed by this pull request

otel metric , log #61

Closed

Icarus9913 force-pushed the feat/wk/otel-metrics branch from 96467b1 to 32161fb Compare April 1, 2022 08:06

weizhoublue reviewed Apr 9, 2022

View reviewed changes

Icarus9913 force-pushed the feat/wk/otel-metrics branch from 32161fb to e9ee503 Compare April 11, 2022 09:23

Icarus9913 commented Apr 11, 2022

View reviewed changes

weizhoublue reviewed Apr 13, 2022

View reviewed changes

Icarus9913 force-pushed the feat/wk/otel-metrics branch from e9ee503 to 0fbeb2b Compare April 18, 2022 15:41

Icarus9913 closed this Apr 19, 2022

Icarus9913 reopened this Apr 19, 2022

weizhoublue closed this Apr 19, 2022

weizhoublue reopened this Apr 19, 2022

Icarus9913 force-pushed the feat/wk/otel-metrics branch 2 times, most recently from 94e34bb to a64f45e Compare April 19, 2022 08:26

spidernet-io deleted a comment from weizhoublue Apr 21, 2022

add otel metrics for spiderpool

e6558e2

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

Icarus9913 force-pushed the feat/wk/otel-metrics branch from a64f45e to 2179714 Compare April 21, 2022 16:15

Icarus9913 force-pushed the feat/wk/otel-metrics branch 8 times, most recently from 1dbeba8 to c41344e Compare April 22, 2022 10:01

weizhoublue reviewed Apr 22, 2022

View reviewed changes

spiderpool metrics pkg optimization

3a7b9ad

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

Icarus9913 force-pushed the feat/wk/otel-metrics branch from c41344e to 3a7b9ad Compare April 23, 2022 14:36

Icarus9913 added 2 commits April 23, 2022 22:38

Merge remote-tracking branch 'upstream/main' into feat/wk/otel-metrics

895eab6

# Conflicts: # go.mod # go.sum # vendor/modules.txt

spiderpool otel metrics fix http server close

0a8b6d0

Signed-off-by: Icarus9913 <icaruswu66@qq.com>

Icarus9913 added pr/release/none-required and removed pr/release/feature-new labels Apr 26, 2022

weizhoublue added the pr/not-ready not ready for merging label May 10, 2022

github-actions bot added the pr/stale This pull is inactive label Jul 9, 2022

github-actions bot removed the pr/stale This pull is inactive label Jul 19, 2022

weizhoublue closed this Sep 1, 2022

Icarus9913 deleted the feat/wk/otel-metrics branch March 10, 2023 06:05

add otel metrics for spiderpool #81

add otel metrics for spiderpool #81

Conversation

Icarus9913 commented Mar 29, 2022 • edited

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

weizhoublue commented Mar 30, 2022

Icarus9913 commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue commented Mar 30, 2022 • edited

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

weizhoublue Apr 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

codecov bot commented Apr 18, 2022 • edited

Codecov Report

Icarus9913 commented Apr 19, 2022

weizhoublue commented Apr 21, 2022

weizhoublue Apr 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue Apr 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 23, 2022

github-actions bot commented Jul 9, 2022

Icarus9913 commented Mar 29, 2022 •

edited

weizhoublue commented Mar 30, 2022 •

edited

weizhoublue Apr 13, 2022 •

edited

codecov bot commented Apr 18, 2022 •

edited

weizhoublue Apr 22, 2022 •

edited

weizhoublue Apr 22, 2022 •

edited