feat: update new metrics #64

duyhungtnn · 2023-05-29T08:36:58Z

#56

Changes

duyhungtnn · 2023-05-31T09:04:10Z

Hi @cre8ivejp This PR has finished all changes required in #56, and all current test cases passed.

But I am adding more test cases to cover all the new changes

duyhungtnn · 2023-06-01T08:13:35Z

hi @cre8ivejp this PR is ready for review , please help me to take a look

cre8ivejp · 2023-06-01T07:40:18Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt

+  sizeByte: Int? = null,
+): MetricsEventData {
+  // note: featureTag only available from `GET_EVALUATIONS`
+  val labels = if (featureTag != null) mapOf("tag" to featureTag) else mapOf()


We must set the tag also for REGISTER_EVENTS API.

Hi @cre8ivejp
I was thinking about **"featureTag" could be null and the REGISTER_EVENTS API did not have the tag**_

We register events to a server in batch.
EventInteractor.kt#L89

That means each event in the batch will have a "featureTag"

Did we support featureTag could be an array of String?

Could featureTag have many different values?

Could the SDK's user change the featureTag?

~~On the BKTConfig.kt, featureTag could be null~~ (I was looking in the wrong class ~ The Builder)

I was checking on the iOS repo and JS repo to find a reference

JS Repo didn't track success | error metric on the "register_event" call.
feat: add ApiClient#registerEvents javascript-client-sdk#17 .
So that it could not use to make decisions and references.

iOS Repo tracks the REGISTER_EVENTS API call but didn't set a tag when tracking the success | error metric. EventInteractor.swift#L169

What do you think about this?

On the BKTConfig.kt, featureTag could be null

android-client-sdk/bucketeer/src/main/kotlin/io/bucketeer/sdk/android/BKTConfig.kt

Line 18 in 25b3ed6

val featureTag: String,

BKTConfig.featureTag is non-null, and we throw error when it is not configured.

android-client-sdk/bucketeer/src/main/kotlin/io/bucketeer/sdk/android/BKTConfig.kt

Line 90 in 25b3ed6

require(!this.featureTag.isNullOrEmpty()) { "featureTag is required" }

So featureTag should always exist.

Could featureTag have many different values?
Could the SDK's user change the featureTag?

BKTConfig is a data class, and featureTag is declared as val, so it's immutable.

BKTClient should be recreated when changing the featureTag.

that means we may have a case that will has many featureTag values @yshrsmz

I'm not sure if I understand correctly, but each event should have a featureTag(from BKTConfig) of the time it is recorded. You always use featureTag from BKTConfig.

So register_events request may include many different featureTags, but success/failure metrics event of that register_events request does nothing to do with featureTags inside the request. You can just use BKTConfig.featureTag to create success/failure event.

@duyhungtnn, as @yshrsmz said, we should always use the BKTConfig.featureTag for both cases when creating metric events.

For example, if we have metric events in the DB with the tag android, and before sending them, the app initializes the SDK again using a different tag, android-tv, it will send metric events with different tags, which is ok.

I'm not sure if this clarifies your question, but please let me know if it doesn't.

@cre8ivejp
on your example :

In the DB has metric events with the tag "android"
["tag: android", "tag: android"]
If sending them fails will create an error metric event with the tag "android"

before sending them, the app initializes the SDK again using a different tag, "android-tv"

after that, some metric events were tracked with the new tag "android-tv"

Now in the DB, we have metrics events with tags "android", and "android-tv".
["tag: android", "tag: android", "tag: android-tv", "tag: android-tv"]

Now it is time to "register_events" in batch ... and we may get some errors.

The question: new error metric will be tracked with what tags?

Option 1: Tags cached in the database. One error metric event with the tag "android", and one error metric event with the tag "android-tv"

Option 2: Or tag in the current BKTConfig.featureTag "android-tv"

If 1) we will have more than 1 new error metric event tracked depending on how many unique featured tags
If 2) we will only have one error metric, but the tag only has the current, not the previous cached.

The problem with (2) is, error metrics events have a tag that seems not correct with cached events when they logged into DB

Hope it clearly my question.

Say you instantiated BKTClient with "android-3" featureTag, and you have 2 events in DB.
BKTClient tries to create a register_events request.

{ "events": [ { "eventType": 4, // METRICS "event": { "labels": { "tag": "android" }, // other values go here... } }, { "eventType": 1, // GOAL "event": { "labels": { "tag": "android-2" }, // other values go here... } } ] }

And the response is returned with error(all registration is failed), so you save failure metrics event.

Then after several minutes, BKTClient tries to send another register_events. In this case, we have 3 events in a DB.

The request looks like this.

{ "events": [ { "eventType": 4, // METRICS "event": { "labels": { "tag": "android" }, // other values go here... } }, { "eventType": 1, // GOAL "event": { "labels": { "tag": "android-2" }, // other values go here... } }, { "eventType": 4, // METRICS "event": { // here you save new failure metrics event with `android-3` tag "labels": { "tag": "android-3" }, // other values go here... } } ] }

So feature tag is in each event, and you don't need to care about multiple featureTag in a single labels map.

The question: new error metric will be tracked with what tags?

You should use BKTConfig.featureTag. new event should always use that value.

cre8ivejp · 2023-06-02T00:20:38Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt

+  apiID: ApiID,
+  type: MetricsEventType,
+  // note: only available on success request
+  latencySecond: Long? = null,


I think it will be better if we split the function or refactor it. One for success and another for failure, so we can avoid those checking for latencySecond and sizeByte throwing an exception.

its ready now

cre8ivejp · 2023-06-02T00:21:22Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt

      sdkVersion = BuildConfig.SDK_VERSION,
      metadata = newMetadata(appVersion),
    ),
  )
 }
+internal fun newMetricsEventData(
+  featureTag: String?,


The featureTag shouldn't be optional at this point.

duyhungtnn · 2023-06-02T01:29:53Z

Hi @*cre8ivejp* Thank you for reviewing. Let me check your comments and make an update.

…

On Fri, Jun 2, 2023, 7:22 AM Alessandro Yuichi Okimoto < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt <#64 (comment)> : > sdkVersion = BuildConfig.SDK_VERSION, metadata = newMetadata(appVersion), ), ) } +internal fun newMetricsEventData( + featureTag: String?, + apiID: ApiID, + type: MetricsEventType, + // note: only available on success request + latencySecond: Long? = null, + // note: only available on success request + sizeByte: Int? = null, +): MetricsEventData { + // note: featureTag only available from `GET_EVALUATIONS` + val labels = if (featureTag != null) mapOf("tag" to featureTag) else mapOf() We must set the tag also for REGISTER_EVENTS API. ------------------------------ In bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt <#64 (comment)> : > sdkVersion = BuildConfig.SDK_VERSION, metadata = newMetadata(appVersion), ), ) } +internal fun newMetricsEventData( + featureTag: String?, + apiID: ApiID, + type: MetricsEventType, + // note: only available on success request + latencySecond: Long? = null, I think it will be better if we split the function or refactor it. One for success and another for failure, so we can avoid those checking for latencySecond and sizeByte throwing an exception. ------------------------------ In bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt <#64 (comment)> : > sdkVersion = BuildConfig.SDK_VERSION, metadata = newMetadata(appVersion), ), ) } +internal fun newMetricsEventData( + featureTag: String?, The featureTag shouldn't be optional at this point. — Reply to this email directly, view it on GitHub <#64 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAT2GTS24FQJAUM3BXEY6E3XJEW45ANCNFSM6AAAAAAYSQMLH4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

yshrsmz · 2023-06-01T08:30:27Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/model/MetricsEventData.kt

-  data class GetEvaluationLatencyMetricsEvent(
+  @JsonClass(generateAdapter = true)
+  data class LatencyMetricsEvent(
+    val apiID: ApiID,


Suggested change

val apiID: ApiID,

val apiId: ApiID,

(not sure if API is updated though)

thanks , I updated that

yshrsmz · 2023-06-02T01:37:37Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt

FYI here's an implementation from the JavaScript SDK

https://github.com/bucketeer-io/javascript-client-sdk/blob/main/src/internal/event/EventCreators.ts

…ent metric

cre8ivejp · 2023-06-02T13:32:36Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventInteractor.kt

+  private fun trackSendEventsFailure(
+    error: BKTException,
+  ) = trackMetricsEventWhenRequestAPIFailure(
+    featureTag = null,


We should use the BKTConfig.featureTag as discussed here.

duyhungtnn · 2023-06-02T14:19:22Z

Thank you @yshrsmz @cre8ivejp I will use the BKConfig.FeatureTag value and finalize the PR

…

On Fri, Jun 2, 2023, 9:13 PM Yasuhiro SHIMIZU ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventCreators.kt <#64 (comment)> : > sdkVersion = BuildConfig.SDK_VERSION, metadata = newMetadata(appVersion), ), ) } +internal fun newMetricsEventData( + featureTag: String?, + apiID: ApiID, + type: MetricsEventType, + // note: only available on success request + latencySecond: Long? = null, + // note: only available on success request + sizeByte: Int? = null, +): MetricsEventData { + // note: featureTag only available from `GET_EVALUATIONS` + val labels = if (featureTag != null) mapOf("tag" to featureTag) else mapOf() Say you instantiated BKTClient with "android-3" featureTag, and you have 2 events in DB. BKTClient tries to create a register_events request. { "events": [ { "eventType": 4, // METRICS "event": { "labels": { "tag": "android" }, // other values go here... } }, { "eventType": 1, // GOAL "event": { "labels": { "tag": "android-2" }, // other values go here... } } ]} And the response is returned with error(all registration is failed), so you save failure metrics event. Then after several minutes, BKTClient tries to send another register_events. In this case, we have 3 events in a DB. The request looks like this. { "events": [ { "eventType": 4, // METRICS "event": { "labels": { "tag": "android" }, // other values go here... } }, { "eventType": 1, // GOAL "event": { "labels": { "tag": "android-2" }, // other values go here... } }, { "eventType": 4, // METRICS "event": { // here you save new failure metrics event with `android-3` tag "labels": { "tag": "android-3" }, // other values go here... } } ]} So feature tag is in each event, and you don't need to care about multiple featureTag in a single labels map. — Reply to this email directly, view it on GitHub <#64 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAT2GTWAUMNCZHUKG6LUHIDXJHYJ7ANCNFSM6AAAAAAYSQMLH4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

duyhungtnn · 2023-06-02T15:53:34Z

@cre8ivejp @yshrsmz
I fixed the problem with the error metric tags and updated testcase.
Please help me to review this PR once again.
Thank you.

yshrsmz · 2023-06-06T00:57:16Z

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/di/InteractorModule.kt

@@ -12,6 +12,7 @@ import io.bucketeer.sdk.android.internal.remote.ApiClient

 internal class InteractorModule(
  val mainHandler: Handler,
+  val featureTag: String,


Can you move this parameter to fun eventInteractor? BKTConfig belongs to DataModule, so we want to ensure we use a value from it.

Just like appVersion parameter.

Let me check it @yshrsmz

bucketeer/src/test/kotlin/io/bucketeer/sdk/android/internal/event/EventInteractorTest.kt

Co-authored-by: Yasuhiro SHIMIZU <the.phantom.bane@gmail.com>

duyhungtnn · 2023-06-06T02:57:51Z

hi @yshrsmz I make the changes as per your suggestion.
Please help me to take a look

yshrsmz

One small suggestion.

but overall, looks good to me.

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventInteractor.kt

…droid/internal/event/EventInteractor.kt Co-authored-by: Yasuhiro SHIMIZU <the.phantom.bane@gmail.com>

duyhungtnn · 2023-06-06T14:09:49Z

@yshrsmz I applied your suggestion.
Please continue reviewing . Thank

yshrsmz

thanks! 👍

cre8ivejp

Looks good! Thank you!

duyhungtnn force-pushed the 56-feat/update-new-metrics branch from 98bb3a5 to e32af66 Compare May 30, 2023 14:20

feat: update new metrics

b95a08b

duyhungtnn force-pushed the 56-feat/update-new-metrics branch from a093883 to b95a08b Compare May 31, 2023 04:24

duyhungtnn added 2 commits May 31, 2023 15:49

fix: JSON Adapter testcase

3ede4b8

fix: lintDebug fail

943130e

duyhungtnn added 5 commits June 1, 2023 13:20

feat: add ApiIDAdapterTest.kt

82e3faf

feat: add BKExceptionToMetricEventTest.kt and remove Deprecated metric

c0bdfa2

fix: Lint error > [no-empty-first-line-in-method-block]

b7b1795

feat: update MetricsEventAdapterFactoryTest.kt

c9a119a

feat: change name MetricsEventType.INTERNAL_ERROR to INTERNAL_SDK_ERROR

6735955

cre8ivejp reviewed Jun 2, 2023

View reviewed changes

cre8ivejp requested a review from yshrsmz June 2, 2023 00:22

yshrsmz reviewed Jun 2, 2023

View reviewed changes

duyhungtnn added 7 commits June 2, 2023 08:58

fix: remove comment on EventInteractorTest.kt

0c51824

fix: lint fail

123f909

fix: create 2 methods for generate success event metrics and error ev…

2b3e01f

…ent metric

fix: missing code

236c9ac

fix: change ApiID enum to ApiId enum, params name apiID to apiId

1015824

fix: change params name apiID to apiId

0d5e96c

fix: lint fail

152e69c

cre8ivejp reviewed Jun 2, 2023

View reviewed changes

duyhungtnn added 4 commits June 2, 2023 22:42

fix: missing tag when log send_event fails error metric events

64b7483

fix: lintKotlin fail

a17d2ba

fix: remove unused comment

a10fd34

fix: featureTag nullable

9d91af9

duyhungtnn requested review from yshrsmz and cre8ivejp June 2, 2023 15:56

yshrsmz suggested changes Jun 6, 2023

View reviewed changes

duyhungtnn and others added 2 commits June 6, 2023 09:52

fix: move featureTag params to fun eventInteractor

9f1faaf

chrore: update comment

435d2c7

Co-authored-by: Yasuhiro SHIMIZU <the.phantom.bane@gmail.com>

duyhungtnn requested a review from yshrsmz June 6, 2023 02:57

yshrsmz reviewed Jun 6, 2023

View reviewed changes

bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/event/EventInteractor.kt Outdated Show resolved Hide resolved

duyhungtnn and others added 2 commits June 6, 2023 21:03

fix: change method name bucketeer/src/main/kotlin/io/bucketeer/sdk/an…

4c94aa0

…droid/internal/event/EventInteractor.kt Co-authored-by: Yasuhiro SHIMIZU <the.phantom.bane@gmail.com>

fix: lint fail

4f20c19

duyhungtnn requested a review from yshrsmz June 6, 2023 14:09

yshrsmz approved these changes Jun 6, 2023

View reviewed changes

cre8ivejp approved these changes Jun 8, 2023

View reviewed changes

cre8ivejp merged commit 61c574b into bucketeer-io:main Jun 8, 2023

bucketeer-bot mentioned this pull request Jun 8, 2023

chore(main): release 2.1.0 #28

Merged

duyhungtnn mentioned this pull request Jun 16, 2023

feat: update iOS SDK bucketeer-io/ios-client-sdk#5

Merged

6 tasks

duyhungtnn deleted the 56-feat/update-new-metrics branch June 3, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update new metrics #64

feat: update new metrics #64

duyhungtnn commented May 29, 2023 •

edited

Loading

duyhungtnn commented May 31, 2023

duyhungtnn commented Jun 1, 2023

cre8ivejp Jun 1, 2023

duyhungtnn Jun 2, 2023 •

edited

Loading

yshrsmz Jun 2, 2023 •

edited

Loading

yshrsmz Jun 2, 2023

duyhungtnn Jun 2, 2023

yshrsmz Jun 2, 2023

cre8ivejp Jun 2, 2023

duyhungtnn Jun 2, 2023 •

edited

Loading

yshrsmz Jun 2, 2023

yshrsmz Jun 2, 2023 •

edited

Loading

cre8ivejp Jun 2, 2023

duyhungtnn Jun 2, 2023

cre8ivejp Jun 2, 2023

duyhungtnn commented Jun 2, 2023 via email

yshrsmz Jun 1, 2023

duyhungtnn Jun 2, 2023

yshrsmz Jun 2, 2023

cre8ivejp Jun 2, 2023

duyhungtnn Jun 2, 2023

duyhungtnn commented Jun 2, 2023 via email •

edited

Loading

duyhungtnn commented Jun 2, 2023

yshrsmz Jun 6, 2023

duyhungtnn Jun 6, 2023

duyhungtnn commented Jun 6, 2023

yshrsmz left a comment

duyhungtnn commented Jun 6, 2023

yshrsmz left a comment

cre8ivejp left a comment

feat: update new metrics #64

feat: update new metrics #64

Conversation

duyhungtnn commented May 29, 2023 • edited Loading

Changes

duyhungtnn commented May 31, 2023

duyhungtnn commented Jun 1, 2023

Choose a reason for hiding this comment

duyhungtnn Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

yshrsmz Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyhungtnn Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yshrsmz Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyhungtnn commented Jun 2, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyhungtnn commented Jun 2, 2023 via email • edited Loading

duyhungtnn commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyhungtnn commented Jun 6, 2023

yshrsmz left a comment

Choose a reason for hiding this comment

duyhungtnn commented Jun 6, 2023

yshrsmz left a comment

Choose a reason for hiding this comment

cre8ivejp left a comment

Choose a reason for hiding this comment

duyhungtnn commented May 29, 2023 •

edited

Loading

duyhungtnn Jun 2, 2023 •

edited

Loading

yshrsmz Jun 2, 2023 •

edited

Loading

duyhungtnn Jun 2, 2023 •

edited

Loading

yshrsmz Jun 2, 2023 •

edited

Loading

duyhungtnn commented Jun 2, 2023 via email •

edited

Loading