chore: login metric added #40325
chore: login metric added #40325
Conversation
WalkthroughThis set of changes introduces comprehensive login metrics tracking into the authentication flow. New constants for login-related metrics are defined, and a new filter is added to record login attempts and failures using Micrometer. The authentication failure handlers are updated to accept a metrics registry and to record failure events with detailed tags. The rate limiting filter is also enhanced to record metrics and invoke the new error handling logic. Security configuration is updated to wire in these new components and ensure metrics are collected for both successful and failed login attempts, including those blocked by rate limiting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginMetricsFilter
participant LoginRateLimitFilter
participant WebFilterChain
participant AuthenticationFailureHandlerCE
participant MeterRegistry
User->>LoginMetricsFilter: HTTP login request
LoginMetricsFilter->>MeterRegistry: Start timer for LOGIN_ATTEMPT
LoginMetricsFilter->>LoginRateLimitFilter: Pass request
LoginRateLimitFilter->>WebFilterChain: Pass request if not rate-limited
WebFilterChain-->>LoginMetricsFilter: Success/Failure
alt Success
LoginMetricsFilter->>MeterRegistry: Stop timer and record LOGIN_ATTEMPT
else Failure
LoginMetricsFilter->>MeterRegistry: Record LOGIN_FAILURE with tags
end
alt Rate limit exceeded
LoginRateLimitFilter->>AuthenticationFailureHandlerCE: handleErrorRedirect
AuthenticationFailureHandlerCE->>MeterRegistry: Record LOGIN_FAILURE with tags
LoginRateLimitFilter->>User: Redirect with error
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LoginSpan.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/AuthenticationFailureHandler.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java(9 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginMetricsFilter.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LoginSpan.java (1)
LoginSpan(5-8)
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginMetricsFilter.java (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java (1)
Slf4j(15-50)app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java (1)
Slf4j(24-97)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LoginSpan.java (1)
LoginSpan(5-8)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java (1)
Slf4j(24-97)app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginMetricsFilter.java (1)
Slf4j(14-47)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LoginSpan.java (1)
LoginSpan(5-8)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (23)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LoginSpan.java (1)
1-8: Clean implementation of login metric constantsThe implementation follows good practices with clear constant naming that aligns with existing span naming conventions by using the
APPSMITH_SPAN_PREFIX. The choice of suffixes (login_failureandlogin_total) is descriptive and reflects the metrics' purposes.app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/AuthenticationFailureHandler.java (1)
5-14: Constructor properly updated for metrics integrationThe constructor has been correctly modified to accept and pass the
MeterRegistryto the parent class, enabling metrics collection for authentication failures.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java (2)
11-12: Appropriate imports for login metrics constantsThese static imports correctly reference the newly defined span constants.
33-35: Correctly configured metric tag preservationThe
seriesExceptionMaphas been properly extended to include the login metrics with their relevant tags. This ensures that important context information like source and error messages are preserved for these metrics while still applying the filter's general tag removal policy to other metrics.app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java (3)
3-8: Appropriate imports for metrics integrationAll necessary imports have been added to support the metrics functionality.
Also applies to: 11-12, 21-21
29-39: Constructor properly updated for metrics integrationThe constructor has been correctly updated to include the
MeterRegistryandAuthenticationFailureHandlerCEdependencies, following proper dependency injection patterns.
79-89: Metrics implementation follows best practicesThe metrics counter is correctly incremented with appropriate tags to provide context about the rate limit failure.
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginMetricsFilter.java (1)
1-47: Well-designed metrics filter implementationThe implementation effectively captures both timing and error information for login attempts. It properly uses Micrometer's Timer.Sample for measuring durations and adds appropriate tags to metrics.
A minor consideration: very long error messages used as tag values could potentially cause storage issues with some metric systems. Consider truncating error messages to a reasonable length.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java (5)
4-5: Good import additions for metricsProper imports added for MeterRegistry and LOGIN_FAILURE constant.
Also applies to: 13-14
8-9: Appropriate OAuth2 exception importCorrectly imported OAuth2AuthenticationException to differentiate between authentication sources.
20-20: Proper dependency injectionGood use of the final keyword for dependency injection with @requiredargsconstructor.
24-32: Well-implemented failure source identificationThe code effectively differentiates between OAuth2 and form login failures, extracting the appropriate error code and incrementing metrics with relevant tags.
36-49: Good implementation of error redirect handlingThe handleErrorRedirect method properly checks for the "error=true" parameter before recording metrics and uses appropriate tags for source and message.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (10)
7-7: Correct imports for new and updated componentsAppropriately imported the new LoginMetricsFilter and updated AuthenticationFailureHandlerCE.
Also applies to: 15-16
23-23: Added MeterRegistry importProperly imported MeterRegistry for metrics collection.
75-86: CE URL constants importsAdded imports for CE version URL constants. Appears to have some duplicate imports with lines 64-74, but this may be intentional for CE/EE code structure.
106-106: Updated authentication failure handler typeChanged from the interface to the specific CE implementation which includes metrics functionality.
140-142: Added MeterRegistry dependencyCorrectly added MeterRegistry for dependency injection.
144-157: Comment formatting improvementsUpdated comment formatting for better readability.
205-209: Comment formatting improvementsUpdated comment formatting for clarity.
214-215: Comment formatting improvementsUpdated comment formatting for clarity.
251-258: Properly integrated metrics filter in the security chainThe LoginMetricsFilter is correctly added before the form login filter, and LoginRateLimitFilter constructor properly updated to include both meterRegistry and authenticationFailureHandler.
315-316: Comment formatting improvementsUpdated comment formatting for better readability.
Also applies to: 332-333
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java
Outdated
Show resolved
Hide resolved
...main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginMetricsFilter.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/LoginRateLimitFilter.java
Outdated
Show resolved
Hide resolved
...main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java
Show resolved
Hide resolved
Description
Added custom login metrics
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14617262543
Commit: e36ee2c
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 23 Apr 2025 12:57:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores