RUMM-664: Shopist demo runs #225

buranmert · 2020-08-17T20:16:14Z

What and why?

We need Shopist app to generate RUM demo data

How?

The app was implemented, this PR implements its UI tests and CI configuration to run them

Also this PR contains linter error fixes in Shopist project
⚠️ In order to make review easier, you can filter "Shopist CI setup is done" commit ⚠️

Visuals

CartViewController has resources, traces, actions, errors and logs ✅

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add more events (eg: user actions, resources, spans) to Shopist app

ncreated

What about other Shopist scenarios? I can see at least 6 distinct stories in Android Shopist Demo.

There is a TODO left in Shopist.xcconfig:

// TODO: RUMM-664 temporary value, otherwise test cases fail in CI
RUM_APPLICATION_ID=12345 // use your own RUM Application ID obtained on datadoghq.com

Shopist/ShopistUITests/ShopistUITests.swift

Shopist/Makefile

Shopist/ShopistUITests/ShopistUITests.swift

buranmert · 2020-08-20T09:49:19Z

~~as one last thing i'm trying to add fake computeCart spans to the app~~
~~apart from that, every page has user actions, resources and random errors now~~

computeCart spans are added ✅

ncreated

Can't review more, as this branch doesn't build:

]

Changing the PR to "draft".

Shopist/Shopist/API.swift

Shopist/Shopist/Extensions/UIImageView+Network.swift

ncreated

Few inline feedbacks and two questions:

what about the IP randomization? I think we need a JIRA to address this.
when will we configure the shopist_demo trigger? I think we need a JIRA to address this.

bitrise.yml

ncreated · 2020-09-02T08:25:40Z

Shopist/Shopist/API.swift

-            if let someError = response.error {
+
+        let tracer = Global.sharedTracer
+        let span = tracer.startSpan(operationName: request.url?.path ?? "network request").setActive()


This usage of setActive() span is against the intention and will produce wrong traces if several requests are started simultaneously.

As we want the "network request" span to be the parent of "decoding response data" span, we should either:

set it explicitly: tracer.startSpan(operationName: "decoding response data", childOf: span.context),

use setRootSpan().setActive() API introduced in RUMM-675 Add tracer.startRootSpan() API #236 .

👍 setRootSpan wasn't available during dev of this PR

ncreated · 2020-09-02T08:30:42Z

Shopist/Shopist/API.swift

+            rum?.startResourceLoading(
+                resourceName: resourceName,
+                url: fontURL,
+                httpMethod: .POST


GET seems more natural for fetching the font.

ncreated · 2020-09-02T08:45:16Z

Shopist/Shopist/AppDelegate.swift

@@ -42,6 +75,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate {

        // Register global `RUMMonitor`
        Global.rum = RUMMonitor.initialize()
+        rum?.addAttribute(forKey: "usr.handle", value: user.email)


Why do we need this?

historical reasons i guess. when android did their shopist, rum-events wasn't around and they used usr.handle instead. sales people still seem to look for it (although they can get the same info from usr.email now)

OK, clear 👌. Maybe it's worth adding the comment above that coded, so this can be removed in the future once everyone sells it by usr.email 🙌.

ncreated · 2020-09-02T08:45:48Z

Shopist/Shopist/AppDelegate.swift

-fileprivate(set) var logger: Logger!
+internal fileprivate(set) var logger: Logger! // swiftlint:disable:this implicitly_unwrapped_optional
+internal let appConfig = AppConfig(serviceName: "ios-sdk-shopist-app")
+internal var rum: DDRUMMonitor? { Global.rum }


Global.rum is no longer optional. Also - do we need this? Why not using Global.rum everywhere?

👍 Global.rum wasn't around when this PR started, i used global rum variable first and it stayed. we can replace them now

ncreated · 2020-09-02T08:48:25Z

Shopist/Shopist/AppDelegate.swift

        // Initialize Datadog SDK
        Datadog.initialize(
            appContext: .init(),
            configuration: Datadog.Configuration
                .builderUsing(
                    rumApplicationID: appConfig.rumAppID,
-                    clientToken: appConfig.rumClientToken,
+                    clientToken: appConfig.clientToken,
                    environment: "tests"


Let's use shop.ist environment name as Android does.

shop.ist is an illegal service name, apparently . is invalid char
i make it shopist 👍

So we have a different definition of valid env between platforms. This must be unified. I created JIRA, so this will be addressed later:

RUMM-700 Investigate the env name validation in Android RUM and iOS RUM

ncreated · 2020-09-02T08:49:53Z

Shopist/Shopist/AppDelegate.swift

@@ -7,29 +7,62 @@
 import UIKit
 import Datadog

-fileprivate(set) var logger: Logger!
+internal fileprivate(set) var logger: Logger! // swiftlint:disable:this implicitly_unwrapped_optional
+internal let appConfig = AppConfig(serviceName: "ios-sdk-shopist-app")


The serviceName defined here is not used anywhere, the demo data shows sh.shopist as the service. Let's use io.shopist.ios as a counterpart to Android:

👍 now appConfig.serviceName (io.shopist.ios) is used in DatadogConfiguration

ncreated · 2020-09-02T08:53:38Z

Shopist/Shopist/Extensions/UIImageView+Network.swift

+        // randomizing cacheKey forces to make new request
+        let cacheKey = UUID().uuidString


Can't we use Alamofire's in-build caching API to disable caching? Defining empty AutoPurgingImageCache should do the same, but it's using regular API instead of hacking it.

AF docs says we should use config.urlCache = nil while init'ing ImageDownloader
done ✅

ncreated · 2020-09-02T09:17:38Z

Shopist/Shopist/Scenes/CheckoutViewController.swift

+
+    override func viewDidAppear(_ animated: Bool) {
+        super.viewDidAppear(animated)
+        viewDidAppearDate = Date()
    }

    override func viewWillDisappear(_ animated: Bool) {
        super.viewWillDisappear(animated)
        rum?.stopView(viewController: self)


When Checkout RUM View gets closed, the underlying view is not started, resulting with no active RUM View until some navigation action is taken:

buranmert · 2020-09-03T09:29:14Z

@ncreated

what about the IP randomization?

we have a ticket for that ✅

when will we configure the shopist_demo trigger?

based on Android team's experience, this should be an almost-trivial task.
i'll keep the current jira in progress until trigger is done, sounds okay?

ncreated

👍

buranmert requested a review from a team as a code owner August 17, 2020 20:16

buranmert self-assigned this Aug 17, 2020

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch from 2d6f7d9 to 84c3676 Compare August 17, 2020 20:21

ncreated requested changes Aug 18, 2020

View reviewed changes

Shopist/ShopistUITests/ShopistUITests.swift Outdated Show resolved Hide resolved

Shopist/ShopistUITests/ShopistUITests.swift Outdated Show resolved Hide resolved

Shopist/ShopistUITests/ShopistUITests.swift Outdated Show resolved Hide resolved

Shopist/Makefile Outdated Show resolved Hide resolved

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch 2 times, most recently from 52694bf to 9e6cba9 Compare August 18, 2020 13:05

ncreated reviewed Aug 19, 2020

View reviewed changes

Shopist/ShopistUITests/ShopistUITests.swift Outdated Show resolved Hide resolved

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch from 9e6cba9 to e8e5675 Compare August 19, 2020 16:55

buranmert marked this pull request as draft August 19, 2020 16:58

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch 5 times, most recently from 9601551 to c59efc8 Compare August 20, 2020 09:46

buranmert marked this pull request as ready for review August 20, 2020 09:46

buranmert requested a review from ncreated August 20, 2020 09:46

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch 7 times, most recently from 0d00100 to 7c2ebd3 Compare August 21, 2020 15:16

ncreated requested changes Aug 24, 2020

View reviewed changes

Shopist/Shopist/API.swift Show resolved Hide resolved

Shopist/Shopist/API.swift Outdated Show resolved Hide resolved

Shopist/Shopist/API.swift Outdated Show resolved Hide resolved

Shopist/Shopist/Extensions/UIImageView+Network.swift Outdated Show resolved Hide resolved

ncreated marked this pull request as draft August 24, 2020 10:03

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch 4 times, most recently from a9af852 to 41f9c3b Compare August 24, 2020 13:54

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch from 41f9c3b to e69ab29 Compare August 24, 2020 13:56

buranmert marked this pull request as ready for review August 24, 2020 13:56

buranmert requested a review from ncreated August 24, 2020 15:34

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch from e69ab29 to fe00b1a Compare August 25, 2020 09:23

buranmert marked this pull request as draft August 25, 2020 11:47

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch 10 times, most recently from c495993 to 19f5911 Compare August 31, 2020 18:19

buranmert marked this pull request as ready for review September 1, 2020 10:00

ncreated requested changes Sep 2, 2020

View reviewed changes

buranmert added 3 commits September 2, 2020 11:35

RUMM-664 Linter errors fixed

14292ea

RUMM-664 Shopist CI setup is done

367b72e

RUMM-664 PR comments addressed

168a065

buranmert force-pushed the buranmert/RUMM-664-shopist-ci branch from 19f5911 to 168a065 Compare September 2, 2020 18:42

buranmert requested a review from ncreated September 2, 2020 18:44

ncreated approved these changes Sep 3, 2020

View reviewed changes

buranmert merged commit 8fbb21b into rum Sep 3, 2020

buranmert deleted the buranmert/RUMM-664-shopist-ci branch September 3, 2020 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-664: Shopist demo runs #225

RUMM-664: Shopist demo runs #225

buranmert commented Aug 17, 2020 •

edited

Loading

ncreated left a comment •

edited by buranmert

Loading

buranmert commented Aug 20, 2020 •

edited

Loading

ncreated left a comment

ncreated left a comment

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 2, 2020

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 2, 2020

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 3, 2020

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 2, 2020

buranmert Sep 2, 2020

ncreated Sep 2, 2020

buranmert commented Sep 3, 2020

ncreated left a comment

		// randomizing cacheKey forces to make new request
		let cacheKey = UUID().uuidString

RUMM-664: Shopist demo runs #225

RUMM-664: Shopist demo runs #225

Conversation

buranmert commented Aug 17, 2020 • edited Loading

What and why?

How?

Visuals

Review checklist

ncreated left a comment • edited by buranmert Loading

Choose a reason for hiding this comment

buranmert commented Aug 20, 2020 • edited Loading

ncreated left a comment

Choose a reason for hiding this comment

ncreated left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buranmert commented Sep 3, 2020

ncreated left a comment

Choose a reason for hiding this comment

buranmert commented Aug 17, 2020 •

edited

Loading

ncreated left a comment •

edited by buranmert

Loading

buranmert commented Aug 20, 2020 •

edited

Loading