Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COSI-19, COSI-21: Enhance Scality COSI Driver with S3 and IAM Metrics, Logging, and IAM Client Improvements #85

Open
wants to merge 5 commits into
base: feature/COSI-65-instrument-cosi-drover-with-gprc-metrics
Choose a base branch
from

Conversation

anurag4DSB
Copy link
Collaborator

@anurag4DSB anurag4DSB commented Jan 2, 2025

This pull request introduces enhancements to the Scality COSI driver, including metrics for IAM and S3 operations, improved logging, and updates to the IAM client initialization process. Key highlights include:
Metrics and Observability:

  • Added Prometheus metrics for IAM and S3 operations, such as request counts and durations for actions like CreateBucket, DeleteBucket, CreateUser, and DeleteUser.
  • Updated /metrics endpoint documentation to include the new metrics and usage examples.

IAM Client Enhancements:

  • Integrated Prometheus middleware for tracking IAM request metrics.
  • Improved clarity and structure by renaming and refactoring key IAM client methods.
  • Enhanced the initialization process with context support.

Testing and Validation:

  • Updated unit and integration tests to validate metrics and IAM operations.
  • Added e2e tests to ensure compatibility with Helm deployments and real-world scenarios.

These changes improve observability, robustness, and clarity in the Scality COSI driver’s operation and metrics tracking.

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.42%. Comparing base (e4b65e3) to head (8880371).

Files with missing lines Patch % Lines
pkg/clients/iam/iam_client.go 57.14% 2 Missing and 1 partial ⚠️
pkg/clients/s3/s3_client.go 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
pkg/driver/provisioner_server_impl.go 97.64% <100.00%> (ø)
pkg/metrics/metrics.go 95.23% <100.00%> (+9.52%) ⬆️
pkg/metrics/prometheus_middleware.go 100.00% <100.00%> (ø)
pkg/clients/iam/iam_client.go 98.14% <57.14%> (-1.86%) ⬇️
pkg/clients/s3/s3_client.go 94.11% <40.00%> (-5.89%) ⬇️
Components Coverage Δ
🏠 Main Package ∅ <ø> (∅)
🚗 Driver Package 92.22% <100.00%> (ø)
📡 gRPC Factory Package 86.25% <ø> (ø)
🔐 IAM Client Package 98.14% <57.14%> (-1.86%) ⬇️
🌐 S3 Client Package 94.11% <40.00%> (-5.89%) ⬇️
🔧 Util Package 100.00% <ø> (ø)
🔖 Constants Package ∅ <ø> (∅)
@@                                     Coverage Diff                                      @@
##           feature/COSI-65-instrument-cosi-drover-with-gprc-metrics      #85      +/-   ##
============================================================================================
- Coverage                                                     93.67%   93.42%   -0.26%     
============================================================================================
  Files                                                            10       11       +1     
  Lines                                                           680      745      +65     
============================================================================================
+ Hits                                                            637      696      +59     
- Misses                                                           37       41       +4     
- Partials                                                          6        8       +2     

@anurag4DSB anurag4DSB force-pushed the feature/COSI-19-add-s3-iam-metrics branch from 7e94e8d to d9b904f Compare January 2, 2025 14:15
@anurag4DSB anurag4DSB force-pushed the feature/COSI-19-add-s3-iam-metrics branch from d9b904f to 8880371 Compare January 2, 2025 14:21
@anurag4DSB anurag4DSB changed the title COSI-19: refactor; use shared ctx in init clients COSI-19: Enhance Scality COSI Driver with Metrics, Logging, and IAM Client Improvements Jan 2, 2025
@anurag4DSB anurag4DSB changed the title COSI-19: Enhance Scality COSI Driver with Metrics, Logging, and IAM Client Improvements COSI-19, COSI-21: Enhance Scality COSI Driver with Metrics, Logging, and IAM Client Improvements Jan 2, 2025
@anurag4DSB anurag4DSB marked this pull request as ready for review January 2, 2025 14:48
@anurag4DSB anurag4DSB changed the title COSI-19, COSI-21: Enhance Scality COSI Driver with Metrics, Logging, and IAM Client Improvements COSI-19, COSI-21: Enhance Scality COSI Driver with S3 and IAM Metrics, Logging, and IAM Client Improvements Jan 2, 2025
Name: "s3_requests_total",
Help: "Total number of S3 requests, categorized by method and status.",
},
[]string{"method", "status"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the S3 operation type is commonly called action throughout the product metrics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will change that.
Thanks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for iam


| Metric Name | Description | Labels | Example Values |
|---------------------------------------------------|------------------------------------------------------------|-------------------|----------------------------------|
| `scality_cosi_driver_iam_request_duration_seconds`| Histogram of IAM request durations in seconds. | `method`, `status`| `CreateUser`, `success`, `error`|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to mention only one of success or error in the example values

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in the table below for IAM Operations note that it is the method label

And add another table for the status label with success and error


| Metric Name | Description | Labels | Example Values |
|---------------------------------------------------|------------------------------------------------------------|-------------------|----------------------------------|
| `scality_cosi_driver_s3_request_duration_seconds` | Histogram of S3 request durations in seconds. | `method`, `status`| `CreateBucket`, `success`, `error`|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto about mentioning only success or error

Comment on lines +126 to +129
if [[ "$CREATE_BUCKET_COUNT" -ne "$EXPECTED_CREATE_BUCKET" ]]; then
echo "Error: CreateBucket count mismatch. Found: $CREATE_BUCKET_COUNT, Expected: $EXPECTED_CREATE_BUCKET" | tee -a "$LOG_FILE"
exit 1
fi
Copy link

@BourgoisMickael BourgoisMickael Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated 4 times, could be a function

validate 'CreateBucket' $CREATE_BUCKET_COUNT $EXPECTED_CREATE_BUCKET $CREATE_BUCKET_DURATION


1. Verify the bucket exists.
2. Use the `DeleteBucket` operation to delete the bucket. Only empty bucket deletion is supported.

## Additional Resource

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here update link to deprecated lib

Copy link

@BourgoisMickael BourgoisMickael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 using aws sdk middleware looks nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants