monitoring: metrics enhancements and proposal for dropping expvar #351

shipperizer · 2023-10-12T23:08:26Z

The aim of this PR is to introduce new metrics for the tcp response time but also to propose a new way of pulling metrics from the LDAP server struct, without using expvar

ldap_metric is fetched on a schedule from the ldap.Server.Stats struct which is how the underlying library instrument its calls, time window is 15s

tcp_response_time_seconds instead is achieved by exploiting the named return parameters and running a defer function in each LDAP operation handler

Example of new metrics added below

# HELP ldap_metric ldap_metric
# TYPE ldap_metric gauge
ldap_metric{library="github.com/glauth/glauth",type="binds"} 21
ldap_metric{library="github.com/glauth/glauth",type="conns"} 21
ldap_metric{library="github.com/glauth/glauth",type="searches"} 20
ldap_metric{library="github.com/glauth/glauth",type="unbinds"} 21

# HELP tcp_response_time_seconds tcp_response_time_seconds
# TYPE tcp_response_time_seconds histogram
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.005"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.01"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.025"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.05"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.1"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.25"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="0.5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="1"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="2.5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="10"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="0",le="+Inf"} 20
tcp_response_time_seconds_sum{library="github.com/glauth/glauth",operation="bind",status="0"} 0.001742719
tcp_response_time_seconds_count{library="github.com/glauth/glauth",operation="bind",status="0"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.005"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.01"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.025"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.05"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.1"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.25"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="0.5"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="1"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="2.5"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="5"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="10"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="bind",status="49",le="+Inf"} 1
tcp_response_time_seconds_sum{library="github.com/glauth/glauth",operation="bind",status="49"} 6.9e-05
tcp_response_time_seconds_count{library="github.com/glauth/glauth",operation="bind",status="49"} 1
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.005"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.01"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.025"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.05"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.1"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.25"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="0.5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="1"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="2.5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="5"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="10"} 20
tcp_response_time_seconds_bucket{library="github.com/glauth/glauth",operation="search",status="0",le="+Inf"} 20
tcp_response_time_seconds_sum{library="github.com/glauth/glauth",operation="search",status="0"} 0.0027715150000000004
tcp_response_time_seconds_count{library="github.com/glauth/glauth",operation="search",status="0"} 20

feat: introduce new prometheus monitor object
feat: add LDAPMonitorWatcher as a potential replacement for v0 Collector
feat: pass monitor object as dependency and instrument core operations
feat: instantiate monitor in main

codesee-maps · 2023-10-12T23:09:08Z

👇 Click on the image for a new way to code review

Legend

Fusion · 2023-10-14T18:22:01Z

Some more healthy separation of concerns.
Glad I'm using Codesee to track everything!

Fusion · 2023-10-14T18:35:07Z

@shipperizer It looks lime at CI time, we have a failure because mockgen was not run.

Fusion · 2023-10-14T18:36:22Z

Also, it looks like we should be moving to Uber's fork of the Mock framework.

reason is due to the following happening in the GetStats method of the ldap.Stats struct ``` internal/monitoring/mock_interfaces.go:92:13: assignment copies lock value to ret0: (github.com/nmcclain/ldap.Stats, bool) contains github.com/nmcclain/ldap.Stats contains sync.Mutex internal/monitoring/mock_interfaces.go:93:9: return copies lock value: github.com/nmcclain/ldap.Stats contains sync.Mutex internal/monitoring/ldap_test.go:23:56: call of mockLDAPServer.EXPECT().GetStats().MinTimes(1).Return copies lock value: github.com/nmcclain/ldap.Stats contains sync.Mutex ```

shipperizer · 2023-10-14T21:32:40Z

@shipperizer It looks lime at CI time, we have a failure because mockgen was not run.

managed to fix that, i forgot to hook the go generate ./...

Also, it looks like we should be moving to Uber's fork of the Mock framework.

happy to move to any mock, main pro of the built-in is the mockgen command, if Uber has the same i'm happy to swap

I noticed that there is a vendored folder that kinda clashes with the right functioning of the go mod vendor command, will have a look if that can be fixed somehow ---> #355

Fusion · 2023-10-16T01:49:52Z

Actually, I was referring to Uber's fork of mockgen as it appears that the original, which you have been using, was EOL'd in July of this year and they recommend switching to that maintained fork.

sonarcloud · 2023-10-16T08:32:24Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
5.5% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

shipperizer · 2023-10-16T08:32:47Z

Actually, I was referring to Uber's fork of mockgen as it appears that the original, which you have been using, was EOL'd in July of this year and they recommend switching to that maintained fork.

@Fusion my bad, didnt really follow that update

should be fully addressed in here

SuperQ · 2023-10-23T17:06:11Z

Hmm, this metric setup is a bit odd and doesn't really follow best practices.

For Go applications, it's discouraged to create metrics in a separate package. Rather you define metrics to be package local. This makes it easier to understand the use and avoids leaking data between packages.

Fusion · 2023-10-24T03:30:29Z

Hi @SuperQ thanks for sharing your knowledge. Could you point us to a bit of literature on this topic?
In the document you linked, I can see "Instantiate the metric classes in the same file you use them" for Prometheus instrumentation but this does not address Go recommendations.

SuperQ · 2023-10-24T05:27:49Z

"Instantiate the metric classes in the same file you use them"

This exactly for Go as well as other languages. You can see a good example in Prometheus itself. Note how the package vars are not exported. This avoids package level metric leaking. It's much easier for both users and developers to know by default the scope of a metric is within a specific section of the code.

I also noticed that there's a 15 second loop, this is also against the implementation practices. If you use a gauge metric where it's used you can avoid having the goroutine updating the data internally.

shipperizer · 2023-10-30T14:16:29Z

I also noticed that there's a 15 second loop, this is also against the implementation practices. If you use a gauge metric where it's used you can avoid having the goroutine updating the data internally.

@SuperQ agreed, unfortunately i tried to reuse the original struct where those "metrics" are created https://github.com/nmcclain/ldap/blob/master/server.go#L66

being in another package meant that to expose those i can only go this way, or otherwise stick with the previous approach of owning internal counters

anuyway point taken, will try to improve on that

shipperizer changed the title ~~monitoring enhancements~~ Monitoring: metrics enhancements and proposal for dropping expvar Oct 12, 2023

shipperizer changed the title ~~Monitoring: metrics enhancements and proposal for dropping expvar~~ monitoring: metrics enhancements and proposal for dropping expvar Oct 12, 2023

shipperizer force-pushed the prometheus branch 4 times, most recently from 78e4d4e to 64dd452 Compare October 13, 2023 14:07

Fusion approved these changes Oct 14, 2023

View reviewed changes

shipperizer added 4 commits October 14, 2023 21:57

feat: introduce new prometheus monitor object

bf2374e

feat: add LDAPMonitorWatcher as a potential replacement for v0 Collector

e0c9533

feat: pass monitor object as dependency and instrument core operations

d8563fa

feat: instantiate monitor in main

44ab9cb

shipperizer force-pushed the prometheus branch from 32e2757 to 459d136 Compare October 14, 2023 21:05

shipperizer added 3 commits October 14, 2023 22:10

ci: exclude mock files

b57d7a2

ci: generate mocks before running tests and go vet

7aa966d

shipperizer force-pushed the prometheus branch from 459d136 to a7539fa Compare October 14, 2023 21:30

deps: move to use go.uber.org/mock/gomock

0cb68c6

Fusion merged commit 94fe62c into glauth:master Oct 21, 2023
7 of 9 checks passed

shipperizer deleted the prometheus branch October 30, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitoring: metrics enhancements and proposal for dropping expvar #351

monitoring: metrics enhancements and proposal for dropping expvar #351

shipperizer commented Oct 12, 2023 •

edited

codesee-maps bot commented Oct 12, 2023 •

edited

Legend

Fusion commented Oct 14, 2023

Fusion commented Oct 14, 2023

Fusion commented Oct 14, 2023

shipperizer commented Oct 14, 2023 •

edited

Fusion commented Oct 16, 2023

sonarcloud bot commented Oct 16, 2023

shipperizer commented Oct 16, 2023 •

edited

SuperQ commented Oct 23, 2023

Fusion commented Oct 24, 2023

SuperQ commented Oct 24, 2023 •

edited

shipperizer commented Oct 30, 2023

monitoring: metrics enhancements and proposal for dropping expvar #351

monitoring: metrics enhancements and proposal for dropping expvar #351

Conversation

shipperizer commented Oct 12, 2023 • edited

codesee-maps bot commented Oct 12, 2023 • edited

Legend

Fusion commented Oct 14, 2023

Fusion commented Oct 14, 2023

Fusion commented Oct 14, 2023

shipperizer commented Oct 14, 2023 • edited

Fusion commented Oct 16, 2023

sonarcloud bot commented Oct 16, 2023

shipperizer commented Oct 16, 2023 • edited

SuperQ commented Oct 23, 2023

Fusion commented Oct 24, 2023

SuperQ commented Oct 24, 2023 • edited

shipperizer commented Oct 30, 2023

shipperizer commented Oct 12, 2023 •

edited

codesee-maps bot commented Oct 12, 2023 •

edited

shipperizer commented Oct 14, 2023 •

edited

shipperizer commented Oct 16, 2023 •

edited

SuperQ commented Oct 24, 2023 •

edited