add constant labels for the metrics (#385) #386

qdongxu · 2023-07-21T14:45:12Z

No description provided.

qdongxu · 2023-07-21T14:56:09Z

Looks like there's an issue in gopkd.in/[email protected] .

with config:

http: # http probes
  - name: EaseProbe Github
    url: https://github.com/megaease/easeprobe
    labels:
      abc: xx
      def: y
      instance: github.com/megaeas
  - name: 163
    url: https://www.163.com

the lablels of 'EaseProbe Github' binds to '163' as well:

EaseProbe_http_status{abc="xx", def="y", endpoint="https://github.com/megaease/easeprobe", exported_instance="github.com/megaeas", instance="localhost:8181", job="easeprobe", name="EaseProbe Github"}
1
EaseProbe_http_status{abc="xx", def="y", endpoint="https://www.163.com", exported_instance="github.com/megaeas", instance="localhost:8181", job="easeprobe", name="163"}
1

qdongxu · 2023-07-21T14:59:33Z

After changing the order:

http: # http probes
  - name: 163
    url: https://www.163.com
  - name: EaseProbe Github
    url: https://github.com/megaease/easeprobe
    labels:
      abc: xx
      def: y
      instance: github.com/megaeas

the labels was overwritten into empty:

EaseProbe_http_status{endpoint="https://github.com/megaease/easeprobe", instance="localhost:8181", job="easeprobe", name="EaseProbe Github"}
1
EaseProbe_http_status{endpoint="https://www.163.com", instance="localhost:8181", job="easeprobe", name="163"}
1

codecov-commenter · 2023-07-24T01:13:42Z

Codecov Report

Patch coverage: 92.42% and project coverage change: -0.26% ⚠️

Comparison is base (5b3a33c) 99.68% compared to head (149c052) 99.42%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   99.68%   99.42%   -0.26%     
==========================================
  Files          83       84       +1     
  Lines        5722     5769      +47     
==========================================
+ Hits         5704     5736      +32     
- Misses         12       25      +13     
- Partials        6        8       +2

Files Changed	Coverage Δ
metric/prometheus.go	`90.35% <67.64%> (-9.65%)`	⬇️
probe/base/base.go	`98.02% <73.33%> (-1.98%)`	⬇️
conf/constlables.go	`100.00% <100.00%> (ø)`
probe/base/metrics.go	`100.00% <100.00%> (ø)`
probe/host/basic.go	`100.00% <100.00%> (ø)`
probe/host/cpu.go	`100.00% <100.00%> (ø)`
probe/host/disk.go	`100.00% <100.00%> (ø)`
probe/host/load.go	`100.00% <100.00%> (ø)`
probe/host/mem.go	`100.00% <100.00%> (ø)`
probe/http/http.go	`100.00% <100.00%> (ø)`
... and 9 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

qdongxu · 2023-07-24T12:21:20Z

This was caused by the strategy of Prometheus, the Registry requires each metric ( identified by the metric name) should have the same label names. prometheus/client_golang#155 .

that is the config should be as below:

http: # http probes
  - name: 163
    url: https://www.163.com
    labels:
      abc: x163
      def: y163
      instance: 163
  - name: EaseProbe Github
    url: https://github.com/megaease/easeprobe
    labels:
      abc: xx
      def: y
      instance: megaease

but since the golang map is unordered, the configuration should be like below:

http: # http probes
  - name: 163
    url: https://www.163.com
    labels:
      - name: abc
         value: x163
     - name: def
       value: y163
    - name: instance
      value: 163
  - name: EaseProbe Github
    url: https://github.com/megaease/easeprobe
    labels:
      - name: abc
         value: xx
     - name: def
       value:  y
    - name: instance
      value: megaease

this looks verbose and error-prone.

samanhappy · 2023-08-01T04:13:28Z

Thanks for contributing this PR, there is a week from last commit, is it ready for review now?

qdongxu · 2023-08-10T06:15:45Z

@samanhappy , the solution is a little ugly. As per the Prometheus Collector constraint, a metric must have the same set of constant labels, which looks like below, all prober should define the three labels: abc, def, instance:

http: # http probes
  - name: 163
    url: https://www.163.com
    labels:
      - name: abc
         value: x163
     - name: def
       value: y163
    - name: instance
      value: 163
  - name: EaseProbe Github
    url: https://github.com/megaease/easeprobe
    labels:
      - name: abc
         value: xx
     - name: def
       value:  y
    - name: instance
      value: megaease

I've tried to use a collector for each prober but didn't find a way to bind all the collectors to one HTTP handler.

Anyway, this meets my reqirement and I am deploying it to the production environment. The code is ready for review if this solution is acceptable.

samanhappy · 2023-08-10T08:10:03Z

You're correct. The current solution is overly verbose and susceptible to errors. We should strive to discover a more effective approach.

Is it possible to consolidate these customized labels into a singular Prometheus label field? This would enable us to maintain label consistency.

However, I'm uncertain about the ease of management within Prometheus consumers like Kibana. What are your thoughts on this?

localvar · 2023-08-10T08:15:27Z

this solution requires all metrics to have the same set of constant labels, but users may forget this requirement and get an unexpected result.

is it possible that we merge the constant labels of all metrics to one set, and set the value of a label to empty if a metric does not need it while keep the configured value if the metric need it?

…te docs

qdongxu · 2023-08-15T16:51:30Z

This PR is ready for review. the merge solution works fine. it passes the collector's checking and the empty labels are discarded by the exporter.

localvar · 2023-08-16T00:51:44Z

docs/Manual.md

+    labels:
+      - name: team
+        value: ease
+      - name: owner
+        value: megaease


as we merged all constant labels into one set, using a map instead of an array will be more user friendly.

Suggested change

labels:

- name: team

value: ease

- name: owner

value: megaease

labels:

team: ease

owner: megaease

qdongxu · 2023-08-17T00:54:22Z

@samanhappy @localvar this PR is ready for review .

localvar · 2023-08-17T01:09:34Z

conf/conf.go

@@ -436,7 +436,9 @@ func isProbe(t reflect.Type) bool {
 // AllProbers return all probers
 func (conf *Conf) AllProbers() []probe.Prober {
 	log.Debugf("--------- Process the probers settings ---------")
-	return allProbersHelper(*conf)
+	all := allProbersHelper(*conf)
+	MergeConstLabels(all)


AllProbers may be called more than once (even it is not currently, the name suggest it could be), which result in MergeConstLabels be called more than once, this is not necessary and may cause a bug in the future.

propose moving MergeConstLabels to somewhere that will only be called once.

localvar · 2023-08-17T01:12:53Z

conf/constlables.go

+	"github.com/megaease/easeprobe/probe"
+)
+
+var constLabels = make(map[string]bool)


why it is a global variable? I think we can make it a local variable by refactor the code below, this could make the code more maintainable and avoid potential bugs.

this was a digress of an original design. updated.

localvar · 2023-08-17T01:15:58Z

metric/prometheus.go

+	for k, v := range constLabels {
+		labels[k] = v
+	}


should check if a label exists both in labels and constLabels and report the error.

localvar

I'll approve this PR now, but please fix the github workflow errors.

qdongxu · 2023-08-17T07:49:35Z

fixed. but I cannot verify fully as some unit tests failed on mac m1 due to the bou.ke/monkey issue. please run a github workflow to verify it.

samanhappy

LGTM，please fix all the code lint warnings and some verbose comments

samanhappy · 2023-08-18T00:59:38Z

conf/constlables.go

+//
+//	Prometheus requires all metric  of the same name have the same set of labels in a collector


Suggested change

//

// Prometheus requires all metric of the same name have the same set of labels in a collector

// Prometheus requires all metric of the same name have the same set of labels in a collector

samanhappy · 2023-08-18T01:01:48Z

conf/constlables.go

+func MergeConstLabels(ps []probe.Prober) {
+	var constLabels = make(map[string]bool)
+	for _, p := range ps {
+		for k, _ := range p.LabelMap() {


please fix this and the following code lint warnings

samanhappy · 2023-08-18T01:02:45Z

metric/prometheus.go


+	// registries[len(registries)-1].MustRegister(m)


Suggested change

// registries[len(registries)-1].MustRegister(m)

Xu Dongyan added 4 commits August 15, 2023 22:08

add constant labels for the metrics (megaease#385)

bfc7c43

[fix] all probers of a probing type should have the same labels

503c362

[fix] all probers of a probing type should have the same labels, upda…

afb32cd

…te docs

merge const labels of all probers

edba6ff

qdongxu force-pushed the labels branch from be187e8 to edba6ff Compare August 15, 2023 15:53

fix constLabels for shell and ssh probe

1965a9f

qdongxu marked this pull request as ready for review August 15, 2023 16:51

localvar reviewed Aug 16, 2023

View reviewed changes

suchen-sci requested a review from localvar August 16, 2023 03:05

Xu Dongyan and others added 8 commits August 16, 2023 13:29

change lables configuration from slice to map

9428e39

remove redundent define of Labels

999da10

fix unit test

8b4b522

refactor

fa9d68e

fix nil bug

3330d3d

fix unit test

a9849d0

fix unit test

79864b9

fix nil bug

b682634

qdongxu mentioned this pull request Aug 16, 2023

fix unix test compiling failed on MAC M1 #401

Closed

format imports

ae1eac3

localvar reviewed Aug 17, 2023

View reviewed changes

qdongxu added 2 commits August 17, 2023 10:25

check duplicate lables

d5b98bd

refacotr for label merge

5f893d4

localvar approved these changes Aug 17, 2023

View reviewed changes

localvar requested review from samanhappy, xxx7xxxx and suchen-sci August 17, 2023 02:52

qdongxu added 3 commits August 17, 2023 11:00

conform to the github code lint warning

ef29e36

conform to the easegress code reivew rules

bbb1a29

fix unit test

2ac06e3

samanhappy approved these changes Aug 18, 2023

View reviewed changes

reformat the code

149c052

zhao-kun approved these changes Aug 23, 2023

View reviewed changes

zhao-kun merged commit b44d1ad into megaease:main Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add constant labels for the metrics (#385) #386

add constant labels for the metrics (#385) #386

qdongxu commented Jul 21, 2023

qdongxu commented Jul 21, 2023

qdongxu commented Jul 21, 2023

codecov-commenter commented Jul 24, 2023 •

edited

Loading

qdongxu commented Jul 24, 2023

samanhappy commented Aug 1, 2023

qdongxu commented Aug 10, 2023 •

edited

Loading

samanhappy commented Aug 10, 2023 •

edited

Loading

localvar commented Aug 10, 2023

qdongxu commented Aug 15, 2023

localvar Aug 16, 2023

qdongxu Aug 16, 2023

qdongxu commented Aug 17, 2023 •

edited

Loading

localvar Aug 17, 2023

qdongxu Aug 17, 2023

localvar Aug 17, 2023

qdongxu Aug 17, 2023

localvar Aug 17, 2023

qdongxu Aug 17, 2023

localvar left a comment

qdongxu commented Aug 17, 2023

samanhappy left a comment

samanhappy Aug 18, 2023

qdongxu Aug 19, 2023

samanhappy Aug 18, 2023

qdongxu Aug 19, 2023 •

edited

Loading

samanhappy Aug 18, 2023

qdongxu Aug 19, 2023

		//
		// Prometheus requires all metric of the same name have the same set of labels in a collector

add constant labels for the metrics (#385) #386

add constant labels for the metrics (#385) #386

Conversation

qdongxu commented Jul 21, 2023

qdongxu commented Jul 21, 2023

qdongxu commented Jul 21, 2023

codecov-commenter commented Jul 24, 2023 • edited Loading

Codecov Report

qdongxu commented Jul 24, 2023

samanhappy commented Aug 1, 2023

qdongxu commented Aug 10, 2023 • edited Loading

samanhappy commented Aug 10, 2023 • edited Loading

localvar commented Aug 10, 2023

qdongxu commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdongxu commented Aug 17, 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

localvar left a comment

Choose a reason for hiding this comment

qdongxu commented Aug 17, 2023

samanhappy left a 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

qdongxu Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 24, 2023 •

edited

Loading

qdongxu commented Aug 10, 2023 •

edited

Loading

samanhappy commented Aug 10, 2023 •

edited

Loading

qdongxu commented Aug 17, 2023 •

edited

Loading

qdongxu Aug 19, 2023 •

edited

Loading