Task/learner 11425 freetier change - #30
Closed
srajupusapati wants to merge 5 commits into
Closed
Conversation
- setup - service name - user id set - config change
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are concrete correctness/build risks (legacy login success URL matching, ineffective tracing configuration, and likely Android-only Datadog dependencies added to a JVM dataseeding module).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the app’s authentication and default-domain behavior to align with Instructure’s OAuth domain migration to sso.canvaslms.com, and introduces Datadog instrumentation across networking and the Student app for observability.
Changes:
- Replace several default Canvas/OAuth domains from
canvas.instructure.comtosso.canvaslms.comand update login redirect URI handling. - Persist and consume the
canvas_regionfield from the OAuth token response. - Add Datadog dependencies and wire Datadog into OkHttp clients and Student app initialization/user identity.
File summaries
| File | Description |
|---|---|
| libs/pandautils/src/main/java/com/instructure/pandautils/utils/Const.kt | Updates profile host constant to the new domain. |
| libs/login-api-2/src/main/java/com/instructure/loginapi/login/api/MobileVerifyAPI.kt | Updates Mobile Verify base URL to sso.canvaslms.com and minor formatting. |
| libs/login-api-2/src/main/java/com/instructure/loginapi/login/activities/BaseLoginSignInActivity.kt | Expands redirect URL matching and changes redirect_uri to the new OAuth domain; stores canvasRegion. |
| libs/login-api-2/src/main/java/com/instructure/loginapi/login/activities/BaseLoginFindSchoolActivity.kt | Changes default domain when user input is empty. |
| libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/utils/ApiPrefs.kt | Adds persisted preference for canvas_region. |
| libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/models/OAuthToken.kt | Adds canvas_region to OAuth token response model. |
| libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/CanvasRestAdapter.kt | Adds Datadog OkHttp interceptor/event listener to shared clients. |
| libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/apis/ErrorReportAPI.kt | Updates default error-report domain to sso.canvaslms.com. |
| libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/apis/AccountDomainAPI.kt | Updates account domain lookup default base domain. |
| libs/canvas-api-2/build.gradle | Adds Datadog OkHttp/Trace dependencies to the shared API library. |
| automation/dataseedingapi/src/main/kotlin/com/instructure/dataseeding/util/CanvasNetworkAdapter.kt | Refactors OkHttp client creation and adds Datadog wiring. |
| automation/dataseedingapi/build.gradle | Adds Datadog dependencies to the dataseeding JVM module. |
| apps/student/src/main/java/com/instructure/student/util/AppManager.kt | Initializes Datadog (logs/rum/tracing), adds redaction mapping, and configures RUM/logging. |
| apps/student/src/main/java/com/instructure/student/fragment/DashboardFragment.kt | Sets Datadog user info during Firebase init. |
| apps/student/build.gradle | Adds Datadog BuildConfig fields and Datadog SDK dependencies. |
| apps/buildSrc/src/main/java/GlobalDependencies.kt | Adds Datadog Gradle plugin coordinate. |
| apps/build.gradle | Adds Datadog Gradle plugin to buildscript classpath. |
Review details
Suppressed comments (2)
apps/student/src/main/java/com/instructure/student/util/AppManager.kt:231
Traceis an annotation type; callingTrace.equals(...)here doesn't apply the TraceConfiguration, so the SpanEventMapper (and URL redaction) will never be used.
Trace.equals(
TraceConfiguration.Builder().setEventMapper(object : SpanEventMapper {
override fun map(event: SpanEvent): SpanEvent {
val originalUrl = event.resource
event.resource = redactSensitiveData(originalUrl)
automation/dataseedingapi/src/main/kotlin/com/instructure/dataseeding/util/CanvasNetworkAdapter.kt:62
- This module is built as a JVM utility (not Android), so wiring DatadogInterceptor/DatadogEventListener into the OkHttp client builder is likely to fail due to missing Android runtime/classes.
val builder = OkHttpClient.Builder()
.retryOnConnectionFailure(retryOnConnectionFailure)
.addInterceptor(DatadogInterceptor.Builder(listOf("*.com")).build())
.eventListenerFactory(DatadogEventListener.Factory())
.addInterceptor(getLoggingInterceptor())
- Files reviewed: 17/17 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+52
| /* Datadog */ | ||
| implementation 'com.datadoghq:dd-sdk-android-okhttp:3.8.0' | ||
| implementation("com.datadoghq:dd-sdk-android-trace:3.8.0") |
Comment on lines
+22
to
+23
| import com.datadog.android.okhttp.DatadogEventListener | ||
| import com.datadog.android.okhttp.DatadogInterceptor |
Comment on lines
67
to
+71
| OkHttpClient.Builder() | ||
| .addNetworkInterceptor(PactRequestInterceptor(authUser)) | ||
| .addNetworkInterceptor(ResponseInterceptor()) | ||
| .addInterceptor(DatadogInterceptor.Builder(listOf("*.com")).build()) | ||
| .eventListenerFactory(DatadogEventListener.Factory()) |
Comment on lines
173
to
+177
| OkHttpClient.Builder() | ||
| .addInterceptor(loggingInterceptor) | ||
| .addInterceptor(RollCallInterceptor()) | ||
| .addInterceptor(DatadogInterceptor.Builder(listOf("*.com")).build()) | ||
| .eventListenerFactory(DatadogEventListener.Factory()) |
Comment on lines
316
to
+320
| .addInterceptor(loggingInterceptor) | ||
| .addInterceptor(RequestInterceptor()) | ||
| .addNetworkInterceptor(ResponseInterceptor()) | ||
| .addInterceptor(DatadogInterceptor.Builder(listOf("*.com")).build()) | ||
| .eventListenerFactory(DatadogEventListener.Factory()) |
Comment on lines
+107
to
+110
| val SUCCESS_URL_COLLECTION = listOf( | ||
| "/canvas/login?code=", //success url | ||
| "login/oauth2/auth?code=" //legacy success url (needed for the fallback redirect_uri) | ||
| ) |
| import java.util.concurrent.TimeUnit | ||
| import javax.inject.Inject | ||
| import com.datadog.android.trace.GlobalDatadogTracer | ||
| import com.datadog.android.trace.Trace |
Comment on lines
221
to
+223
| //if the user enters nothing, try to connect to canvas.instructure.com | ||
| if (url!!.trim { it <= ' ' }.isEmpty()) { | ||
| url = "canvas.instructure.com" | ||
| url = "sso.canvaslms.com" |
Comment on lines
+44
to
+45
| import androidx.core.view.ViewCompat | ||
| import androidx.core.view.WindowInsetsCompat |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: https://2u-internal.atlassian.net/browse/LEARNER-11425
Instructure has migrated their OAuth domain from "https://canvas.instructure.com/" to "https://sso.canvaslms.com/"
Reference: instructure/canvas-android@315c53f