-
Notifications
You must be signed in to change notification settings - Fork 46
Move ests telemetry related code behind a feature flag AB#3352490, Fixes AB#3352490 #2742
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
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a feature flag mechanism to control eSTS telemetry operations for performance optimization. The changes wrap existing telemetry code behind a feature flag check to avoid executing expensive telemetry operations when disabled.
- Added
SKIP_ESTS_TELEMETRYfeature flag to control telemetry execution - Created
TelemetryUtils.executeIfEstsTelemetryEnabled()method to conditionally execute telemetry code - Wrapped all eSTS telemetry calls throughout the codebase with the new utility method
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TelemetryUtils.java | Implements the core feature flag checking logic and execution wrapper |
| CommonFlight.java | Adds the new SKIP_ESTS_TELEMETRY feature flag definition |
| AttributeName.java | Adds telemetry attribute to track when eSTS telemetry is skipped |
| CacheEndEvent.java | Wraps EstsTelemetry.emit() call with feature flag check |
| OAuth2Strategy.java | Wraps EstsTelemetry header retrieval calls with feature flag check |
| NativeAuthRequestProvider.kt | Wraps EstsTelemetry header retrieval with feature flag check |
| CommandDispatcher.java | Wraps multiple EstsTelemetry method calls with feature flag check |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/eststelemetry/TelemetryUtils.java
Outdated
Show resolved
Hide resolved
|
✅ Work item link check complete. Description contains link AB#3352490 to an Azure Boards work item. |
common4j/src/main/com/microsoft/identity/common/java/eststelemetry/TelemetryUtils.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/eststelemetry/TelemetryUtils.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/eststelemetry/TelemetryUtils.java
Outdated
Show resolved
Hide resolved
|
How about we make EstsTelemetry.getInstance() return NoOpEstsTelemetry when flight is enabled? Only change needed would be EstsTelemetry class, instead of updating every call. |
...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/OAuth2Strategy.java
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/OAuth2Strategy.java
Outdated
Show resolved
Hide resolved
mohitc1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
Emitting eSTS telemetry is an expensive operation that impacts the performance of calls. Therefore, we are placing it behind a feature flag to avoid executing telemetry-related operations. AB#3352490