Skip to content

Conversation

@felixgateru
Copy link
Contributor

@felixgateru felixgateru commented Oct 16, 2024

What type of PR is this?

This is a refactor as it groups current service middleware.

What does this do?

This pr creates the following folder structure for magistrala services:
service
-- middleware
---- authorization.go
---- logging.go
---- metrics.go
---- tracing.go
---- doc.go

Which issue(s) does this PR fix/relate to?

Noissue

Have you included tests for your changes?

No

Did you document any new/modified feature?

Yes changes include godoc documentation

Notes

To be merged after #2444

@felixgateru felixgateru force-pushed the noissue-middleware branch 3 times, most recently from be16129 to 8128e0d Compare October 22, 2024 10:03
@codecov
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.66%. Comparing base (0465ceb) to head (ecb8bfa).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/messaging/handler/logging.go 0.00% 1 Missing ⚠️
pkg/messaging/handler/metrics.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2472      +/-   ##
==========================================
+ Coverage   34.49%   43.66%   +9.17%     
==========================================
  Files         318      211     -107     
  Lines       46477    33458   -13019     
==========================================
- Hits        16030    14608    -1422     
+ Misses      29651    18107   -11544     
+ Partials      796      743      -53     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@felixgateru felixgateru marked this pull request as ready for review October 22, 2024 10:21
@felixgateru felixgateru requested a review from a team as a code owner January 20, 2025 11:54
@felixgateru felixgateru force-pushed the noissue-middleware branch 2 times, most recently from f7a9075 to 49da8ce Compare August 7, 2025 15:50
@dborovcanin
Copy link
Collaborator

@felixgateru Please resolve conflicts.

Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Let's also rename factory functions to NewLogging, NewMetrics... so we have middleware.New<middleware type>.

Comment on lines 4 to 8
// Package middleware provides authorization, logging, metrics and tracing middleware
// for Magistrala Clients Service.
//
// For more details about tracing instrumentation for Magistrala messaging refer
// to the documentation at https://docs.magistrala.abstractmachines.fr/tracing/.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is SuperMQ, not Magistrala.

Comment on lines 4 to 8
// Package middleware provides tracing, logging and metrics middleware
// for Magistrala Notifiers service.
//
// For more details about tracing instrumentation for Magistrala messaging refer
// to the documentation at https://docs.magistrala.abstractmachines.fr/tracing/.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, SMQ, not MG.

Comment on lines 4 to 5
// Package middleware provides logging middleware for Magistrala
// Provision service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same. Check other MG references as well.

Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
logger *slog.Logger
}

// NewRoleManagerLoggingMiddleware adds logging facilities to the core service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the same approach: NewLogging (and NewAuth snd NewMetric) here, so we have something like middlewarealias.NewLogging....

Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
return nil, err
}
ram, err := rmMW.NewRoleManagerAuthorizationMiddleware(policies.ChannelType, svc, authz, rolesOpPerm, callout)
ram, err := rmMW.NewAuthorization(policies.ChannelType, svc, authz, rolesOpPerm, callout)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid using mixedCaps in package aliases. Use rolemw here.

Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
Signed-off-by: Felix Gateru <felix.gateru@gmail.com>
@dborovcanin dborovcanin merged commit b031dc0 into absmach:main Sep 26, 2025
7 of 9 checks passed
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