Return eventId from Tracker.track() (close #304) by mscwilson · Pull Request #310 · snowplow/snowplow-java-tracker

Miranda Wilson (mscwilson) · 2022-03-07T15:09:31Z

For issue #304.

These changes remove two inappropriate fields from Events, remove the Hamcrest dependency, and add a return type to Tracker.track().

Bad stuff

Previously, eventId was an Event field. Users were able to provide their own event ID when building an Event, or one would be generated automatically. The eventId was passed to the payload on TrackerPayload creation, when Event.getPayload() was called during Tracker.track() - or before PR #293, during event sending by the Emitter.

Snowplow pipelines rely on eventIds being unique; allowing it to be set manually was very risky.

Users were also still able to set deviceCreatedTimestamp, a legacy option from before the introduction of trueTimestamp. TrueTimestamp is designed specifically for users to set their own timestamp. Aside from that, setting deviceCreatedTimestamp then means it referred to when the Event was built, which isn't necessarily when it's tracked.

New stuff

EventId and deviceCreatedTimestamp are now fields of TrackerPayload, and are generated automatically on TrackerPayload initialisation. DeviceCreatedTimestamp now more accurately describes the time at which Tracker.track() was called.

Previously, users could access an Event's eventId using Event.getEventId() for use elsewhere in their application. We now return the eventId from Tracker.track(). If the payload was lost because the Emitter buffer was full, null is returned instead.

I also removed the Hamcrest test dependency, which was only used in a couple of places.

AlexBenny

Great job 👍

Nice cleaning, better testing, removed a test dependency! 🏆

I added few comments, many of them are just considerations and points of view.

AlexBenny · 2022-03-07T16:33:27Z

+     * A TrackerPayload object - or more than one, in the case of eCommerceTransaction events -
+     * will be created from the Event. This is passed to the configured Emitter.
+     * If the event was successfully added to the Emitter buffer for sending,
+     * a list containing the payload's eventId string (a UUID) is returned.
+     * EcommerceTransactions will return all the relevant eventIds in the list.
+     * If the Emitter event buffer is full, the payload will be lost. In this case, this method
+     * returns a list containing null.


So pitiful that those legacy eCommerce transaction events force us to do this :(
I'm wondering... what if we return just the eventID of the transaction event without the eventID of all the ecommerce items events?
Essentially those item events are like entities tracked as events due to legacy.
BTW, just an idea, let's keep this as you implemented now.

AlexBenny · 2022-03-07T16:53:50Z

        payload.add(Parameter.TR_COUNTRY, this.country);
        payload.add(Parameter.TR_CURRENCY, this.currency);
-        return putDefaultParams(payload);
+        return putTrueTimestamp(payload);


This code is quite ugly because the AbstractEvent tries to cover both types of events: SelfDescribing and atomic (primitive) events.

Otherwise we could use super to pass the values of the abstract class and everything would make much more sense. For example, ScreenView is quite ugly because of this.

A correct distinction between SelfDescribing and Primitive would help to write the code in this way.

public TrackerPayload getPayload() { TrackerPayload payload = super(); payload.add(Parameter.EVENT [...]

and the abstract class would have...

public TrackerPayload getPayload() { TrackerPayload payload = new TrackerPayload(); if (getTrueTimestamp() != null) { payload.add(Parameter.TRUE_TIMESTAMP, Long.toString(getTrueTimestamp())); } return payload; }

Anyway, for now we can't do anything like this. It's ok until we don't change the way the events work in the tracker.

AlexBenny · 2022-03-08T09:21:58Z

+        // this throws an exception if it's not a valid UUID string
+        UUID.fromString(result.get(0));


I think we can make this explicit in the test.
Probably the result would be the same but raising an exception in the tests could have an unexpected behaviour not just like a failure. I can't find any reference to support this and it could be I remember cases related to another platform/language (ObjC ?!) but JUnit offers some assert methods for this so it can be worth to use them: https://www.baeldung.com/junit-assert-exception

Raising an exception does just fail the test, but yeah it would be nicer to assert it explicitly

No need to change this, I misunderstood the test. The comment made me think to an exception testing but actually you are testing it's successful 👍

I've changed it anyway. You misread it so it could have been clearer :)

AlexBenny · 2022-03-08T09:27:22Z

-                .build(), result1);
+                .build();
+
+        assertTrue(result1.entrySet().containsAll(expected1.entrySet()));


👍 👍 👍

AlexBenny · 2022-03-08T09:32:55Z

        Map<String, String> result2 = results.get(1).getMap();
-        assertEquals(ImmutableMap.<String, String>builder()
-                .put("dtm", "123456")
+        Map<String, String> expected2 = ImmutableMap.<String, String>builder()


It seems the expected2 is the same as expected1. Removing dtm and eid there aren't differences.
Probably it's the same case for other tests.

AlexBenny · 2022-03-08T09:37:20Z

        subjectPairs.put("tz", "Etc/UTC");
        subjectPairs.put("cd", "24");
+
        assertEquals(subjectPairs, tracker.getSubject().getSubject());


tracker.getSubject().getSubject() :D

AlexBenny · 2022-03-08T09:38:12Z

-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.*;


AlexBenny · 2022-03-08T09:40:29Z

-        public String getUrl() {
-            return null;
-        }
+        public String getUrl() { return null; }


Just my opinion. I don't see this a coding style mistake. For me it's not that wrong to have the function on multiple lines even with a single line block.

AlexBenny · 2022-03-08T09:45:40Z

+        long currentTime = System.currentTimeMillis();
+        TrackerPayload payload = new TrackerPayload();
+        long timeDifference = payload.getDeviceCreatedTimestamp() - currentTime;
+        assertTrue(timeDifference < 1000);


I guess it could be even less than 1000.
Not a big thing btw.

actually 100 was too low!

Miranda Wilson (mscwilson) added 7 commits March 3, 2022 11:51

Remove deviceCreatedTimestamp and eventId from AbstractEvent builder

8e20735

Set eid and dtm at TrackerPayload creation

50c274f

Remove method calls from Tracker (tests failing)

1ff0165

Add tests to TrackerPayload

fd9c5d0

Remove Hamcrest dependency

1b06ae6

Return eventId from Tracker.track()

eec1ce0

Add eid and dtm to TrackerPayload map directly

3d0b317

Miranda Wilson (mscwilson) requested a review from AlexBenny March 7, 2022 15:09

Snowplow CLA bot (snowplowcla) added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Mar 7, 2022

Miranda Wilson (mscwilson) mentioned this pull request Mar 7, 2022

Return eventId from Tracker.track() #304

Closed

Snowplow (snowplow) deleted a comment from Snowplow CLA bot (snowplowcla) Mar 7, 2022

Miranda Wilson (mscwilson) removed the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Mar 7, 2022

AlexBenny approved these changes Mar 8, 2022

View reviewed changes

Improve clarity of Tracker tests

cfb5d5e

Miranda Wilson (mscwilson) merged commit 753ff45 into release/0.12.0 Mar 8, 2022

Miranda Wilson (mscwilson) deleted the issue/304-remove_dtm_eid_setters branch March 8, 2022 11:15

		// this throws an exception if it's not a valid UUID string
		UUID.fromString(result.get(0));

		import static org.hamcrest.MatcherAssert.assertThat;
		import static org.hamcrest.Matchers.*;

Conversation

Miranda Wilson (mscwilson) commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bad stuff

New stuff

Uh oh!

AlexBenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Miranda Wilson (mscwilson) commented Mar 7, 2022 •

edited

Loading