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.

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

iOS-8282 analytics parameters update #4135

Open
wants to merge 10 commits into
base: releases/5.18
Choose a base branch
from

Conversation

fedorov-d
Copy link
Contributor

Перенес логику обработки ошибок стейкинга в CommonStakingAnalyticsLogger (из-за этого пришлось часть ошибок сделать public). Удалил не используемые в стейкинге ошибки. Удалил лишние эвенты аналитики, добавил недостающие и в некоторые эвенты добавил параметры

@fedorov-d fedorov-d marked this pull request as ready for review November 4, 2024 06:37
@fedorov-d fedorov-d requested review from tureck1y and a team as code owners November 4, 2024 06:37
@tureck1y
Copy link
Collaborator

tureck1y commented Nov 4, 2024

этот закрываем?

@fedorov-d fedorov-d changed the base branch from releases/5.17.2 to releases/5.18 November 5, 2024 07:11
@fedorov-d
Copy link
Contributor Author

этот закрываем?

не не, Леша сказал в 5.18, я сменил base

Comment on lines +18 to +23
if let code = apiError.code {
parameters[.errorCode] = code
}
if let message = apiError.message {
parameters[.errorMessage] = message
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if let не обязателен, можно просто parameters[.errorCode] = apiError.code и т.д.

case let error as LocalizedError where error is StakingManagerError || error is StakeKitMapperError:
parameters[.errorDescription] = error.errorDescription
event = .stakingAppErrors
default: return
Copy link
Contributor

Choose a reason for hiding this comment

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

Все другие ошибки (например системные или любые добавленные в будущем) будут втихую игнорироваться

Прошлая реализация работала по другому

@@ -11,7 +11,7 @@ import SwiftUI

enum TokenNotificationEvent: Hashable {
case networkUnreachable(currencySymbol: String)
case someNetworksUnreachable
case someNetworksUnreachable(networks: [WalletModel])
Copy link
Contributor

@m3g0byt3 m3g0byt3 Nov 5, 2024

Choose a reason for hiding this comment

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

Что-то c большой вероятность где-то утечет, если напрямую в енам класть WalletModel
Это тяжелая модель с кучей подписок и логики внутри, реф тип

Например TokenNotificationEvent.someNetworksUnreachable может бесконечно долго лежать как value в currentSubject, не давая этим wallet model умереть

Мне кажется тут лучше держать айдишники, а не сами walletmodel

@@ -12,7 +12,6 @@ extension Analytics {
enum Event: String {
case signedIn = "[Basic] Signed in"
case toppedUp = "[Basic] Topped up"
case walletOpened = "[Basic] Wallet Opened"
Copy link
Contributor

Choose a reason for hiding this comment

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

А все эти удаленные ивенты чем-то будут заменены?
Просто странно что сами то действия остались (рефреш, перелистывание и т.д.), удалена только их аналитика

@m3g0byt3 m3g0byt3 added the release ASAP to look label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release ASAP to look
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants