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.

Sign up for GitHub

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

Jump to bottom

feat(prover): integrate sentry to monitor error logs & panic events #1477

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

feat(prover): integrate sentry to monitor error logs & panic events #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0bd20e8
integrate sentry
amoylan2 Aug 8, 2024
a1803c1
add partner_name implementation
amoylan2 Aug 8, 2024
376e300
feat: fix sentry issues
amoylan2 Aug 13, 2024
9927cae
refactor version logic
amoylan2 Aug 13, 2024
2956a25
fix lint issues
amoylan2 Aug 13, 2024
bbfbfc5
add custom environment tag to sentry
amoylan2 Aug 13, 2024
09f4830
coordinator issue sentry endpoint
amoylan2 Aug 19, 2024
89d7f6d
add alternative logger
amoylan2 Aug 20, 2024
53b48b8
merge develop & fix conflicts
amoylan2 Aug 22, 2024
1c31cec
move GetEmptyTaskError to inner get_task method
amoylan2 Aug 22, 2024
ee41125
prover get sentry endpoint from coordinator
amoylan2 Aug 22, 2024
c213c0b
fix lint issue
amoylan2 Aug 22, 2024
dd003ad
remove unused lines
amoylan2 Aug 22, 2024
5495a73
filter the case when sentry endpoint is empty string
amoylan2 Aug 22, 2024
52666a7
fix lint issue
amoylan2 Aug 22, 2024
91fe9c8
Merge branch 'develop' into feat/prover_integrate_sentry
georgehao Aug 23, 2024
229eef2
merge develop, fix conflicts
amoylan2 Sep 29, 2024
43fba89
feat(prover): dynamicly detect sentry endpoint from coordinator
amoylan2 Sep 29, 2024
2352802
fix bug
amoylan2 Sep 30, 2024
fa25261
Merge branch 'develop' into feat/prover_integrate_sentry
georgehao Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions coordinator/conf/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
"prover_manager": {
"provers_per_session": 1,
"session_attempts": 5,
"bundle_collection_time_sec": 180,
"batch_collection_time_sec": 180,
"chunk_collection_time_sec": 180,
"verifier": {
"mock_mode": true,
"low_version_circuit": {
Expand All @@ -19,7 +16,11 @@
"fork_name": "darwinV2",
"min_prover_version": "v4.4.45"
}
}
},
"bundle_collection_time_sec": 180,
"batch_collection_time_sec": 180,
"chunk_collection_time_sec": 180,
"prover_sentry_endpoint": ""
},
"db": {
"driver_name": "postgres",
Expand Down
2 changes: 2 additions & 0 deletions coordinator/internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ type ProverManager struct {
ChunkCollectionTimeSec int `json:"chunk_collection_time_sec"`
// BundleCollectionTimeSec bundle Proof collection time (in seconds).
BundleCollectionTimeSec int `json:"bundle_collection_time_sec"`
// ProverSentryEndpoint the sentry endpoint being sent to each prover in login response
ProverSentryEndpoint string `json:"prover_sentry_endpoint"`
Comment on lines +27 to +28
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding URL validation for the Sentry endpoint.

Since this endpoint is critical for error monitoring, consider adding validation in the NewConfig function to ensure the URL is properly formatted when provided.

Here's a suggested implementation:

 func NewConfig(file string) (*Config, error) {
 	buf, err := os.ReadFile(filepath.Clean(file))
 	if err != nil {
 		return nil, err
 	}
 
 	cfg := &Config{}
 	err = json.Unmarshal(buf, cfg)
 	if err != nil {
 		return nil, err
 	}
 
 	// Override config with environment variables
 	err = utils.OverrideConfigWithEnv(cfg, "SCROLL_COORDINATOR")
 	if err != nil {
 		return nil, err
 	}
+
+	// Validate Sentry endpoint if provided
+	if cfg.ProverManager != nil && cfg.ProverManager.ProverSentryEndpoint != "" {
+		if _, err := url.Parse(cfg.ProverManager.ProverSentryEndpoint); err != nil {
+			return nil, fmt.Errorf("invalid prover_sentry_endpoint URL: %v", err)
+		}
+	}
 
 	return cfg, nil
 }

Don't forget to add these imports:

import (
    "fmt"
    "net/url"
)

}

// L2 loads l2geth configuration items.
Expand Down
15 changes: 15 additions & 0 deletions coordinator/internal/controller/api/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ package api
import (
"errors"
"fmt"
"time"

jwt "github.com/appleboy/gin-jwt/v2"
"github.com/gin-gonic/gin"
"gorm.io/gorm"

commonTypes "scroll-tech/common/types"

"scroll-tech/coordinator/internal/config"
"scroll-tech/coordinator/internal/logic/auth"
"scroll-tech/coordinator/internal/logic/verifier"
Expand All @@ -16,12 +19,14 @@ import (

// AuthController is login API
type AuthController struct {
cfg *config.Config
loginLogic *auth.LoginLogic
}

// NewAuthController returns an LoginController instance
func NewAuthController(db *gorm.DB, cfg *config.Config, vf *verifier.Verifier) *AuthController {
return &AuthController{
cfg: cfg,
loginLogic: auth.NewLoginLogic(db, cfg, vf),
}
}
Expand Down Expand Up @@ -98,3 +103,13 @@ func (a *AuthController) IdentityHandler(c *gin.Context) interface{} {

return nil
}

// LoginResponse replies to client for /login
func (a *AuthController) LoginResponse(c *gin.Context, code int, message string, time time.Time) {
resp := types.LoginSchema{
Time: time,
Token: message,
SentryEndpoint: a.cfg.ProverManager.ProverSentryEndpoint,
}
commonTypes.RenderSuccess(c, resp)
}
Comment on lines +108 to +115
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider these improvements to the LoginResponse method.

  1. The time parameter could be generated inside the method instead of being passed as an argument.
  2. Consider adding input validation for the parameters.
  3. Consider using constants for response field names.

Here's a suggested improvement:

-func (a *AuthController) LoginResponse(c *gin.Context, code int, message string, time time.Time) {
+func (a *AuthController) LoginResponse(c *gin.Context, code int, message string) {
+       if c == nil {
+               return
+       }
+       if message == "" {
+               message = "Login successful"
+       }
        resp := types.LoginSchema{
-               Time:           time,
+               Time:           time.Now(),
                Token:          message,
                SentryEndpoint: a.cfg.ProverManager.ProverSentryEndpoint,
        }
        commonTypes.RenderSuccess(c, resp)
}

Committable suggestion was skipped due to low confidence.

2 changes: 1 addition & 1 deletion coordinator/internal/middleware/login_jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func LoginMiddleware(conf *config.Config) *jwt.GinJWTMiddleware {
TokenLookup: "header: Authorization, query: token, cookie: jwt",
TokenHeadName: "Bearer",
TimeFunc: time.Now,
LoginResponse: loginResponse,
LoginResponse: api.Auth.LoginResponse,
})

if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions coordinator/internal/types/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ const (

// LoginSchema for /login response
type LoginSchema struct {
Time time.Time `json:"time"`
Token string `json:"token"`
Time time.Time `json:"time"`
Token string `json:"token"`
SentryEndpoint string `json:"sentry_endpoint,omitempty"`
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding validation and documentation for the SentryEndpoint field.

Since this field contains sensitive configuration information:

  1. Add field documentation explaining its purpose and expected format
  2. Consider implementing validation for the endpoint URL format
  3. Add logging sanitization to prevent the endpoint from being logged

Here's a suggested improvement:

 type LoginSchema struct {
 	Time           time.Time `json:"time"`
 	Token          string    `json:"token"`
-	SentryEndpoint string    `json:"sentry_endpoint,omitempty"`
+	// SentryEndpoint holds the URL for error reporting to Sentry.
+	// This field should not be logged or exposed in debug output.
+	// Format: https://<key>@<organization>.ingest.sentry.io/<project>
+	SentryEndpoint string    `json:"sentry_endpoint,omitempty" sensitive:"true"`
 }

Consider adding a validation method:

// ValidateSentryEndpoint ensures the endpoint URL is properly formatted
func (l *LoginSchema) ValidateSentryEndpoint() error {
    if l.SentryEndpoint == "" {
        return nil
    }
    _, err := url.Parse(l.SentryEndpoint)
    if err != nil {
        return fmt.Errorf("invalid sentry endpoint format: %w", err)
    }
    return nil
}

}

// Message the login message struct
Expand Down
Loading
Loading