COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates #83

anurag4DSB · 2025-01-02T11:05:08Z

The old PR is #69

go.* dependency updates are responsible for 440 line changes.

This PR introduces metrics instrumentation for gRPC API calls and adds comprehensive testing and documentation updates. Key changes include:

Instrumentation: Added Prometheus metrics for gRPC API calls.
Testing:
- Updated unit tests for the run method to include Prometheus registry.
- Added unit tests for metrics and server packages.
- Enhanced e2e tests and Helm deployments to validate metrics functionality.
Helm & Kustomize: Added services for exposing metrics in Helm chart and Kustomize configurations.
Documentation:
- Updated local development documentation to include metrics configuration.
- Added documentation for driver parameters and metrics endpoints.
- Dependency Management: Ran go mod tidy to ensure dependencies are up to date.

These changes provide better observability and make it easier to monitor the driver’s performance and behavior in production environments.

anurag4DSB · 2025-01-02T11:06:03Z

cmd/scality-cosi-driver/cmd.go

+	driverPrefix         = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default cosi.scality.com")
+	driverMetricsAddress = flag.String("driver-metrics-address", defaultMetricsAddress, "The address to expose Prometheus metrics, default: :8080")
+	driverMetricsPath    = flag.String("driver-metrics-path", defaultMetricsPath, "path for the metrics endpoint, default: /metrics")
+	driverMetricsPrefix  = flag.String("driver-custom-metrics-prefix", defaultMetricsPrefix, "prefix for the metrics, default: scality_cosi_driver_")


for now a place holder, will be used for S3 and IAM metrics

codecov · 2025-01-02T11:09:19Z

Codecov Report

Attention: Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.67%. Comparing base (9c4e7f0) to head (e4b65e3).

Files with missing lines	Patch %	Lines
pkg/metrics/metrics.go	85.71%	2 Missing and 1 partial ⚠️
pkg/grpcfactory/server.go	96.15%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
pkg/grpcfactory/server.go	`94.23% <96.15%> (+10.89%)`	⬆️
pkg/metrics/metrics.go	`85.71% <85.71%> (ø)`

Components	Coverage Δ
🏠 Main Package	`∅ <ø> (∅)`
🚗 Driver Package	`92.22% <ø> (ø)`
📡 gRPC Factory Package	`86.25% <96.15%> (+4.60%)`	⬆️
🔐 IAM Client Package	`100.00% <ø> (ø)`
🌐 S3 Client Package	`100.00% <ø> (ø)`
🔧 Util Package	`100.00% <ø> (ø)`
🔖 Constants Package	`∅ <ø> (∅)`

@@                             Coverage Diff                              @@
##           improvement/COSI-53-misc-ci-improvements      #83      +/-   ##
============================================================================
+ Coverage                                     93.40%   93.67%   +0.26%     
============================================================================
  Files                                             9       10       +1     
  Lines                                           637      680      +43     
============================================================================
+ Hits                                            595      637      +42     
- Misses                                           36       37       +1     
  Partials                                          6        6

Update unit test for run methods usage to include prometheus registry.

anurag4DSB · 2025-01-02T13:35:06Z

pkg/metrics/metrics.go

+var (
+	S3RequestsTotal    *prometheus.CounterVec
+	S3RequestDuration  *prometheus.HistogramVec
+	IAMRequestsTotal   *prometheus.CounterVec
+	IAMRequestDuration *prometheus.HistogramVec
+)


place holders for s3 and iam metrics

anurag4DSB · 2025-01-02T13:42:39Z

pkg/metrics/metrics_test.go

+var _ = Describe("InitializeMetrics", func() {
+	var (
+		registry          *prometheus.Registry
+		driverMetricsPath string
+	)
+
+	BeforeEach(func() {
+		registry = prometheus.NewRegistry()
+		driverMetricsPath = "/metrics"
+	})
+
+	It("should serve metrics via an HTTP endpoint", func() {
+		addr := "127.0.0.1:0"
+		server, err := metrics.StartMetricsServerWithRegistry(addr, registry, driverMetricsPath)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(server).NotTo(BeNil())
+
+		resp, err := http.Get("http://" + server.Addr + driverMetricsPath)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(resp.StatusCode).To(Equal(http.StatusOK))
+
+		err = server.Close()
+		Expect(err).NotTo(HaveOccurred())
+	})
+})


place holder tests for initialize metrics for s3 and iam

BourgoisMickael · 2025-01-03T13:40:46Z

.github/scripts/e2e_tests_metrics.sh

+log_and_run() {
+  echo "Running: $*" | tee -a "$LOG_FILE"


Not sure I commented this already, but wouldn't a set -x be simpler to print command as well ?

Then the whole script could be tee into a logfile

BourgoisMickael · 2025-01-03T13:45:44Z

.github/scripts/e2e_tests_metrics.sh

+# Wait a few seconds to ensure port-forward is established
+log_and_run sleep 5


You can test the port here.

# wait for $localport to become available while ! nc -vz localhost $localport > /dev/null 2>&1 ; do # echo sleeping sleep 0.1 done # This would show that the port is open # nmap -sT -p $localport localhost

https://stackoverflow.com/a/68824178

BourgoisMickael · 2025-01-03T13:48:51Z

.github/workflows/e2e-feature-tests.yml

@@ -116,6 +116,14 @@ jobs:
      run: |
        .github/scripts/e2e_tests_brownfield_use_case.sh

+    # the script accepts number of requests for  APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO


Suggested change

# the script accepts number of requests for APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO

# the script accepts number of requests for APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO

or is there a missing word ?

BourgoisMickael · 2025-01-03T13:50:05Z

.github/workflows/e2e-feature-tests.yml

+    # Example below we we are testing for 2 CREATE_BUCKET, 1 DELETE_BUCKET,
+    # 1 GET_INFO, 2 GRANT_ACCESS and 2 REVOKE_ACCESS API counts


we we + use list it's easier to read

Suggested change

# Example below we we are testing for 2 CREATE_BUCKET, 1 DELETE_BUCKET,

# 1 GET_INFO, 2 GRANT_ACCESS and 2 REVOKE_ACCESS API counts

# Example below we are testing for those API counts:

# - 2 CREATE_BUCKET

# - 1 DELETE_BUCKET

# - 1 GET_INFO

# - 2 GRANT_ACCESS

# - 2 REVOKE_ACCESS

BourgoisMickael · 2025-01-03T13:51:17Z

.github/workflows/helm-validation.yml

@@ -72,6 +72,14 @@ jobs:
      run: |
        .github/scripts/verify_helm_install.sh

+    # the script accepts number of requests for  APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO


same comment as previous file

BourgoisMickael · 2025-01-03T14:07:07Z

cmd/scality-cosi-driver/cmd.go

+	defaultDriverAddress  = "unix:///var/lib/cosi/cosi.sock"
+	defaultDriverPrefix   = "cosi"
+	defaultMetricsPath    = "/metrics"
+	defaultMetricsPrefix  = "scality_cosi_driver"


Suggested change

defaultMetricsPrefix = "scality_cosi_driver"

defaultMetricsPrefix = "scality_cosi_driver_"

And in the documentation md file as well
or the trailing underscore should be removed from the flag.String

BourgoisMickael · 2025-01-03T14:07:46Z

cmd/scality-cosi-driver/cmd.go

-	driverAddress = flag.String("driver-address", "unix:///var/lib/cosi/cosi.sock", "driver address for the socket")
-	driverPrefix  = flag.String("driver-prefix", "", "prefix for COSI driver, e.g. <prefix>.scality.com")
+	driverAddress        = flag.String("driver-address", defaultDriverAddress, "driver address for the socket file, default: unix:///var/lib/cosi/cosi.sock")
+	driverPrefix         = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default cosi.scality.com")


Suggested change

driverPrefix = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default cosi.scality.com")

driverPrefix = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default: cosi")

BourgoisMickael · 2025-01-03T14:17:49Z

docs/metrics-overview.md

+
+## Additional Resource
+
+- [gRPC-Go Prometheus Metrics](https://github.com/grpc-ecosystem/go-grpc-prometheus)


Suggested change

- [gRPC-Go Prometheus Metrics](https://github.com/grpc-ecosystem/go-grpc-prometheus)

- [gRPC-Go Prometheus Metrics](https://github.com/grpc-ecosystem/go-grpc-middleware)

Change link to the deprecated lib

BourgoisMickael · 2025-01-03T14:24:03Z

pkg/grpcfactory/server.go

+
+	srvMetrics := grpcprom.NewServerMetrics(
+		grpcprom.WithServerHandlingTimeHistogram(
+			grpcprom.WithHistogramBuckets([]float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120}),


Note that histogram can greatly increase cardinality.

If this runs on client side outside our control, maybe it could be interesting to have those buckets configurable so they can change easily is they want more or less cardinality or if they want to target more specifically a certain range

COSI-65: instrument-metrics-for-grpc-api-calls

ad6743b

anurag4DSB commented Jan 2, 2025

View reviewed changes

anurag4DSB force-pushed the feature/COSI-65-instrument-cosi-drover-with-gprc-metrics branch from 1f69f34 to 87f52de Compare January 2, 2025 11:23

anurag4DSB added 4 commits January 2, 2025 12:34

COSI-65: dependency update (go mod tidy)

e8636b8

COSI-65: Update existing unit tests

d46cf29

Update unit test for run methods usage to include prometheus registry.

COSI-65: Add unit tests for metrics and server pkg

102c1be

COSI-65: Test metrics in e2e and helm deployments

eb13820

anurag4DSB force-pushed the feature/COSI-65-instrument-cosi-drover-with-gprc-metrics branch from 87f52de to f5d58e9 Compare January 2, 2025 11:34

anurag4DSB marked this pull request as ready for review January 2, 2025 11:39

anurag4DSB added 4 commits January 2, 2025 13:35

COSI-46: Add service for metrics in helm chart

3f948ce

COSI-46: Add service for metrics in kustomize

3df356f

COSI-65: Update local development docs for metrics

fe8ce0d

COSI-21: Add docs for metrics and driver params

e4b65e3

anurag4DSB force-pushed the feature/COSI-65-instrument-cosi-drover-with-gprc-metrics branch from e48db12 to e4b65e3 Compare January 2, 2025 12:35

anurag4DSB changed the title ~~COSI-65: Metrics Intrumentation + e2e tests~~ COSI-65: Add GRPC Metrics Instrumentation and Documentation Updates Jan 2, 2025

anurag4DSB commented Jan 2, 2025

View reviewed changes

anurag4DSB changed the title ~~COSI-65: Add GRPC Metrics Instrumentation and Documentation Updates~~ COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates Jan 2, 2025

anurag4DSB mentioned this pull request Jan 2, 2025

COSI-65: Add Prometheus Metrics Support/Instrumentation to COSI Driver #69

Closed

anurag4DSB requested review from BourgoisMickael and fredmnl January 2, 2025 18:57

BourgoisMickael reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates #83

COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates #83

anurag4DSB commented Jan 2, 2025 •

edited

Loading

anurag4DSB Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

anurag4DSB Jan 2, 2025

anurag4DSB Jan 2, 2025

BourgoisMickael Jan 3, 2025

BourgoisMickael Jan 3, 2025

BourgoisMickael Jan 3, 2025

BourgoisMickael Jan 3, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025

BourgoisMickael Jan 3, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025

BourgoisMickael Jan 3, 2025

		# Wait a few seconds to ensure port-forward is established
		log_and_run sleep 5

	# the script accepts number of requests for APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO
	# the script accepts number of requests for APIs for CREATE_BUCKET, DELETE_BUCKET, GET_INFO

		# Example below we we are testing for 2 CREATE_BUCKET, 1 DELETE_BUCKET,
		# 1 GET_INFO, 2 GRANT_ACCESS and 2 REVOKE_ACCESS API counts

-    # Example below we we are testing for 2 CREATE_BUCKET, 1 DELETE_BUCKET,
-    # 1 GET_INFO, 2 GRANT_ACCESS and 2 REVOKE_ACCESS API counts
+    # Example below we are testing for those API counts:
+    # - 2 CREATE_BUCKET
+    # - 1 DELETE_BUCKET
+    # - 1 GET_INFO
+    # - 2 GRANT_ACCESS
+    # - 2 REVOKE_ACCESS

	defaultMetricsPrefix = "scality_cosi_driver"
	defaultMetricsPrefix = "scality_cosi_driver_"

	driverPrefix = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default cosi.scality.com")
	driverPrefix = flag.String("driver-prefix", defaultDriverPrefix, "prefix for COSI driver, e.g. <prefix>.scality.com, default: cosi")


		## Additional Resource

		- [gRPC-Go Prometheus Metrics](https://github.com/grpc-ecosystem/go-grpc-prometheus)

COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates #83

Are you sure you want to change the base?

COSI-65, COSI-46, COSI-21: Add GRPC Metrics Instrumentation and Documentation Updates #83

Conversation

anurag4DSB commented Jan 2, 2025 • edited Loading

anurag4DSB Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

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

BourgoisMickael Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BourgoisMickael Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

BourgoisMickael Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anurag4DSB commented Jan 2, 2025 •

edited

Loading

anurag4DSB Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025 •

edited

Loading

BourgoisMickael Jan 3, 2025 •

edited

Loading