[management] only count login request duration for successful logins

[pascal-fischer]

Describe your changes

Issue ticket number and link

Stack

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

• I added/updated documentation for this change
• Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

• Bug Fixes
  • Improved timing accuracy for performance metrics collected during sync and login operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR relocates gRPC metrics timing in the server handler for Sync and Login operations. CountSyncRequestDuration moves post-unlock, while Login's deferred logging is replaced with immediate duration metrics recorded after response encryption.

Changes

Cohort / File(s)	Summary
gRPC Metrics Timing management/internals/shared/grpc/server.go	Repositions CountSyncRequestDuration to execute after peer lock release and subsequent operations. Replaces deferred Login logging with immediate CountLoginRequestDuration and duration log following response encryption.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• [management] Fix sync metrics #4939: Modifies Sync metrics timing in the same file (management/internals/shared/grpc/server.go), adjusting when CountSyncRequestDuration is recorded.

Suggested reviewers

• crn4

Poem

🐰 Metrics dancing to a different beat,
Timing shuffled, now more discrete,
After locks release their hold,
Measurements precise, stories told,
A rabbit's hop through metrics neat! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description uses the template structure but 'Describe your changes' section is empty, and no issue ticket, stack information, or explanation for why documentation isn't needed are provided.	Complete the 'Describe your changes' section with details about moving login metrics timing, add an issue ticket reference, and explain why documentation is not needed for this change.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'only count login request duration for successful logins' directly aligns with the main change of moving gRPC metrics timing specifically for the Login path to only occur after successful encryption.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/login-request-duration-counter

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

🧹 Nitpick comments (1)

management/internals/shared/grpc/server.go (1)

794-797: Consider using peer.AccountID for more accurate metrics.

At this point, accountID may still be "UNKNOWN" if GetAccountIDForPeerKey failed (lines 735-739) but the login succeeded via setup key registration. The peer object returned from LoginPeer contains the actual account ID.

♻️ Suggested improvement

 	if s.appMetrics != nil {
-		s.appMetrics.GRPCMetrics().CountLoginRequestDuration(time.Since(reqStart), accountID)
+		s.appMetrics.GRPCMetrics().CountLoginRequestDuration(time.Since(reqStart), peer.AccountID)
 	}
 	log.WithContext(ctx).Debugf("Login took %s", time.Since(reqStart))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@management/internals/shared/grpc/server.go` around lines 794 - 797, The
metrics currently use the variable accountID which may remain "UNKNOWN" when
GetAccountIDForPeerKey failed even though LoginPeer returned a valid peer with
the real account ID; update the metrics call to use the account ID from the
returned peer (peer.AccountID) when available (e.g., use peer.AccountID fallback
before calling s.appMetrics.GRPCMetrics().CountLoginRequestDuration) and ensure
the debug log likewise reports the peer.AccountID where appropriate; adjust
references around LoginPeer, accountID, peer, and
s.appMetrics.GRPCMetrics().CountLoginRequestDuration to prefer peer.AccountID
when non-empty.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@management/internals/shared/grpc/server.go`:
- Around line 794-797: The metrics currently use the variable accountID which
may remain "UNKNOWN" when GetAccountIDForPeerKey failed even though LoginPeer
returned a valid peer with the real account ID; update the metrics call to use
the account ID from the returned peer (peer.AccountID) when available (e.g., use
peer.AccountID fallback before calling
s.appMetrics.GRPCMetrics().CountLoginRequestDuration) and ensure the debug log
likewise reports the peer.AccountID where appropriate; adjust references around
LoginPeer, accountID, peer, and
s.appMetrics.GRPCMetrics().CountLoginRequestDuration to prefer peer.AccountID
when non-empty.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a154d175-2c20-4fd8-96ab-1054a455f086

📥 Commits

Reviewing files that changed from the base of the PR and between 30c02ab and c22f3ab.

📒 Files selected for processing (1)

• management/internals/shared/grpc/server.go