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

[Feature] 앰플리튜드를 도입합니다. #140

Merged
merged 13 commits into from
Nov 22, 2024
Merged

[Feature] 앰플리튜드를 도입합니다. #140

merged 13 commits into from
Nov 22, 2024

Conversation

librarywon
Copy link
Contributor

@librarywon librarywon commented Nov 21, 2024 •
edited
Loading

resolved #139

AS-IS

X

TO-BE

  • 앰플리튜드를 도입하여 사용자가 어떤 행동을 하고 어느 화면에서 머물고 있는지 확인할 수 있습니다.
  • 열람실 공부시간이 miutes로 들어가있어 이용시간이 이상하게 나오는 현상을 해결하였습니다.
  • ForegroundService의 알람 문구를 수정하였습니다.

KEY-POINT

  • core.tracker 모듈에 구현을 하였고 다른 송모씨의 회사 이야기를 들어보니 여러 트래킹 툴을 쓸 수 있을 것 같아 Tracker라는 이름으로 추상화하였습니다.
  • core.desginSystem에 프로바이더를 배치하여 모든 스크린이 사용할 수 있도록하였습니다.
  • 중간에 autocapture라는 속성은 앰플리튜드가 알아서 오토캡처를 해준다고해서 추가하였습니다.
  autocapture =
                    autocaptureOptions {
                        +sessions
                        +appLifecycles
                        +screenViews
                    },

SCREENSHOT (Optional)

  • 직접 엠플리튜드 사이트에 들어가며 내용을 확인할 수 있습니다.

@librarywon librarywon requested a review from no1msh November 21, 2024 06:44
@librarywon librarywon self-assigned this Nov 21, 2024
no1msh
no1msh approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@no1msh no1msh left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 코멘트 확인해주세요!

app/src/main/AndroidManifest.xml Outdated
@@ -6,7 +6,7 @@
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" />
<uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
<uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC" />

<uses-permission android:name="android.permission.READ_PHONE_STATE" />
Copy link
Contributor

Choose a reason for hiding this comment

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

사용자가 동의 버튼을 눌러야하는 권한인가요?

Copy link
Contributor Author

@librarywon librarywon Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

�이게 최신 앰플리튜드 sdk 가이드에는 퍼미션을 받을 필요가 없어 보여서 제거하겠습니다.

core/designsystem/src/main/java/com/teamhy2/designsystem/util/compositionlocal/LocalTracker.kt Outdated
Comment on lines 6 to 9
val LocalTracker =
compositionLocalOf<Tracker> {
error("No Tracker provided")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

default로 예외를 던져버리면 프리뷰 함수 내에서도 컴포지션 로컬을 provide 해주지 않을 때 미리보기가 안나옵니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

에러를 던질필요가 있을까 싶어서 기본값은 아무것도 던지지 않는 것으로 수정해보겠습니다

gradle/libs.versions.toml
@@ -35,6 +35,7 @@ googleFirebaseCrashlytics = "3.0.2"
datastore = "1.0.0"
firebaseFirestoreKtx = "25.0.0"
coil = "2.3.0"
amplitude = "1.+"
Copy link
Contributor

Choose a reason for hiding this comment

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

+는 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앰플리튜드 공식문서에 이렇게 적혀있어서 그대로 사용했는데 알아서 최신버전을 찾는거 같아요

Copy link
Contributor Author

@librarywon librarywon Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

+가 *동적 버전 지정이라고 해서 최신 버전을 받아오는 것이 맞습니다.

core/tracker/src/main/java/com/teamhy2/tracker/Tracker.kt
Comment on lines +3 to +8
interface Tracker {
fun trackEvent(
eventName: String,
properties: Map<String, Any?> = emptyMap(),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

추상화 좋습니다.
하지만 properties가 다른 tracker를 연결할 때 걸림돌이 되진 않을까여?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

기본값을 명시해서 다른 tracker들은 선택적으로 사용할 수 있도록 설계하였는데 다른 tracker들이 어떤 형태를 가지는지 모르겠어서 예상이 잘 가지 않네요 일단 이 부분은 knownIssue로 가져가는 것을 제안드립니다!

core/tracker/src/main/java/com/teamhy2/tracker/di/TrackerModule.kt Outdated
Comment on lines 16 to 24
object TrackerModule {
@Provides
@Singleton
fun provideTracker(
@ApplicationContext context: Context,
): Tracker {
return AmplitudeTracker(context, AMPLITUDE_KEY)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

AMPLITUDE_KEY라는 상수를 어디서 왔는지 표현하기 위해 다음 처럼 명시하는게 좋아보입니다.
BuildConfig.AMPLITUDE_KEY

@librarywon librarywon merged commit df435fe into develop Nov 22, 2024
1 check passed
@librarywon librarywon deleted the Feature/#139-amplitude branch November 22, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@no1msh no1msh no1msh approved these changes

Assignees

@librarywon librarywon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] 앰플리튜드를 도입합니다.
2 participants
@librarywon @no1msh