Generic web analytics tracking implementation #681

th3M1ke · 2021-01-22T11:10:44Z

Which problem is this PR solving?

resolves generic analytics tracking (in addition to GA) #652

Short description of the changes

GA module was transferred to a separate class and will be invoked once UI configuration property UIConfig.tracking.gaID will be added
if will be provided customWebAnalytics implementation via UI configuration property UIConfig.tracking.customWebAnalytics it will be used as web analytic in jaeger UI
if any of UI configuration will not be provided - empty mock(() => {}) will be used
only one web analytic system can work. Priority: Custom Web Analytic -> Google Analytics -> Mock
UIConfig.tracking.customWebAnalytics format(also described in IWebAnalytics in types):

const CustomWebAnalytics: IWebAnalytics = {
    init: () => {},
    trackPageView: () => {},
    trackError: () => {},
    trackEvent: () => {},
    context: null,
    isEnabled: () => false,
};

UIconfig.js:

function UIConfig() {
  return {
     ...other properties
     tracking: {
       customWebAnalytics: function () {
         return {
           init: () => { /* initialization actions */ },
           trackPageView: (pathname, search) => {},
           trackError: (description) => {},
           trackEvent: (category, action, labelOrValue, value) => {},
           context: null,
           isEnabled: () => false,
         }
       }
     }
  }
}

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

codecov · 2021-01-22T11:19:25Z

Codecov Report

Merging #681 (491353a) into master (48956d5) will increase coverage by 0.12%.
The diff coverage is 93.25%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   94.23%   94.36%   +0.12%     
==========================================
  Files         228      230       +2     
  Lines        5931     5959      +28     
  Branches     1492     1448      -44     
==========================================
+ Hits         5589     5623      +34     
+ Misses        304      302       -2     
+ Partials       38       34       -4

Impacted Files	Coverage Δ
...ackages/jaeger-ui/src/constants/default-config.tsx	`100.00% <ø> (ø)`
packages/jaeger-ui/src/middlewares/track.tsx	`12.50% <0.00%> (-62.50%)`	⬇️
packages/jaeger-ui/src/utils/tracking/utils.tsx	`80.00% <80.00%> (ø)`
packages/jaeger-ui/src/utils/tracking/ga.tsx	`93.84% <93.84%> (ø)`
packages/jaeger-ui/src/utils/tracking/index.tsx	`100.00% <100.00%> (+20.83%)`	⬆️
.../jaeger-ui/src/utils/tracking/noopWebAnalytics.tsx	`100.00% <100.00%> (ø)`
...kages/jaeger-ui/src/model/transform-trace-data.tsx	`90.58% <0.00%> (ø)`
.../jaeger-ui/src/components/common/BreakableText.tsx	`83.33% <0.00%> (ø)`
...ents/TracePage/TraceTimelineViewer/TimelineRow.tsx	`77.77% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48956d5...186e6c8. Read the comment docs.

yurishkuro

it would be good to ramp up some test coverage, 0.19% drop is pretty big.

yurishkuro · 2021-01-22T14:21:26Z

packages/jaeger-ui/src/types/tracking.tsx

@@ -0,0 +1,30 @@
+// Copyright (c) 2021 Uber Technologies, Inc.


Suggested change

// Copyright (c) 2021 Uber Technologies, Inc.

// Copyright (c) 2021 The Jaeger Authors.

Is this header being added automatically?

No, I believe I just copied it from some file...

yurishkuro · 2021-01-22T14:22:08Z

packages/jaeger-ui/src/utils/tracking/ga.tsx

@@ -0,0 +1,187 @@
+// Copyright (c) 2021 Uber Technologies, Inc.


Suggested change

// Copyright (c) 2021 Uber Technologies, Inc.

// Copyright (c) 2021 The Jaeger Authors.

yurishkuro · 2021-01-22T14:28:24Z

packages/jaeger-ui/src/utils/tracking/index.tsx

 }

 export function trackError(description: string) {
-  if (isGaEnabled) {
-    let msg = description;


I'm confused - all this functionality now seems to only exist in the GA implementation of IWebAnalytics, but wasn't it a general functionality that would be applicable to any other implementation? I would've expected only the reporting part to be swapped out, not all the data preparation / massaging.

yurishkuro · 2021-01-22T14:29:25Z

packages/jaeger-ui/src/utils/tracking/index.tsx


-// Util so "0" and "false" become false
-const isTruish = (value?: string | string[]) => Boolean(value) && value !== '0' && value !== 'false';
+  const GenericWebAnalytics: IWebAnalytics = {


maybe NoopWebAnalytics?

yurishkuro · 2021-01-22T14:32:41Z

packages/jaeger-ui/src/constants/default-config.tsx

@@ -85,6 +85,7 @@ export default deepFreeze(
      tracking: {
        gaID: null,
        trackErrors: true,
+        customWebAnalytics: null,


is there a place where the type of this function is defined (it seems to take config as arg)?

If I understand the question correct, my answer is yes it will look like

customWebAnalytics: function (config?) { return { init: () => { /* initialization actions */ }, trackPageView: (pathname, search) => {}, trackError: (description) => {}, trackEvent: (category, action, labelOrValue, value) => {}, context: null, isEnabled: () => false, } }

I meant whether we have a type definition for Config somewhere. Maybe not.

packages/jaeger-ui/src/utils/tracking/index.tsx

yurishkuro · 2021-01-22T14:39:16Z

packages/jaeger-ui/src/utils/tracking/index.tsx

+    webAnalytics = config.tracking.customWebAnalytics(config) as IWebAnalytics;
+  } else if (config.tracking && config.tracking.gaID) {
+    webAnalytics = new GA(config);
+  }


Another option instead of forced priority order is to allow enabling both GA and custom WA via a wrapper implementation that would dispatch events to both. But I don't feel strongly about it, the priority order seems fine.

yurishkuro · 2021-01-22T14:45:26Z

packages/jaeger-ui/src/utils/tracking/index.tsx

-// enable for tests, debug or if in prod with a GA ID
-export const isGaEnabled = isTest || isDebugMode || (isProd && Boolean(gaID));
-const isErrorsEnabled = isDebugMode || (isGaEnabled && Boolean(_get(config, 'tracking.trackErrors')));
+  if (webAnalytics.isEnabled()) {


This feels like an odd ordering, checking a property before the object was initialized. I would call init() unconditionally. It can always check isEnabled internally if necessary, but perhaps in some cases init() would actually initialize that property (e.g by checking if a valid app token is provided in the constructor).

Alternatively, could this not be done directly in the constructor? No additional information is provided to init() anyway.

th3M1ke · 2021-01-25T12:41:59Z

I'm confused - all this functionality now seems to only exist in the GA implementation of IWebAnalytics, but wasn't it a general functionality that would be applicable to any other implementation? I would've expected only the reporting part to be swapped out, not all the data preparation / massaging.

Do you suggest making a transport layer? if so, I do not clearly understand how should it look like. Currently, I lot of logic goes with GA and data preparation to be sent.

let's make alignment about the implementation itself:

We have the next function that currently using:

trackPageView
trackError
trackEvent

trackPageView can be a part of trackEvent?
trackError - can be used for web analytics and, also we can pass the custom logger

Does this fit for you? Or can we reduce them to trackError and trackEvent?

Once we add only the transport layer users can not modify incoming data and map it according to their needs
Is invoking custom WA good from your perspective?

  return {
     ...other properties
     tracking: {
       customWebAnalytics: function () {
         return {
           init: () => { /* initialization actions */ },
           trackPageView: (pathname, search) => {},
           trackError: (description) => {},
           trackEvent: (category, action, labelOrValue, value) => {},
           context: null,
           isEnabled: () => false,
         }
       }
     }
  }
}

Another option instead of forced priority order is to allow enabling both GA and custom WA via a wrapper implementation that would dispatch events to both. But I don't feel strongly about it, the priority order seems fine.

Sure we can init them both in parallel. Wrappers could be implemented inside trackPageView, trackError, trackEvent in packages/jaeger-ui/src/utils/tracking/index.tsx

it would be good to ramp up some test coverage, 0.19% drop is pretty big.

Will add tests as we align about implementation if it is ok :)

yurishkuro · 2021-01-26T01:29:04Z

Do you suggest making a transport layer?

Well, depends on how you look at it. Take the logic in the init method, for example - it determines app version, checks some cookies, configures raven, etc. A good 80-90 lines of code, of which maybe 5 are related to GA. So shouldn't that same functionality be available if you're reporting events to another provider?

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke · 2021-02-11T19:49:32Z

@yurishkuro, I'm not sure about transport. From my perspective transport layers remove flexibility, and add complexity. We need to expose adapters for different analytic systems or provide such ability to users.

About errors. Not all systems provide specific error tracking function. So you have to track them as events. But for errors user can provide a custom logger via configuration.

As for raven, we can provide it as a configuration and the user can decide to use it or not.

yurishkuro · 2021-02-11T19:54:10Z

ok. We can refactor that later, right now it's not worse than it was before, so I'd be ok to merge.

th3M1ke · 2021-02-11T19:58:10Z

🤝 Let me increase test coverage shortly!

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke · 2021-02-15T11:01:18Z

@yurishkuro can you take a look at codecov analysis? https://codecov.io/gh/jaegertracing/jaeger-ui/pull/681/diff

There are gaps under google analytics implementation. Those lines are related to development. Should I also cover them as well?

yurishkuro · 2021-02-15T15:20:31Z

the rule of thumb is to leave the code with the cumulative coverage no worse than before

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro · 2021-02-15T20:22:36Z

seems the checks are stuck

yurishkuro · 2021-02-15T20:35:26Z

https://codecov.io/github/jaegertracing/jaeger-ui/commit/aa603193a1ed5daf343ce4d44ceda2d721ded953

Is it possible to run GA tests in both debug and non-debug mode? This will improve the coverage

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro · 2021-02-16T20:07:39Z

+0.13% https://codecov.io/github/jaegertracing/jaeger-ui/commit/491353a892a932b76b22c67c9a5a8e2e48e3c97b

* Generic web analytics tracking implementation Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update due to comments, refactor Google Analytic tracker Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Add unit tests Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update tests Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update tests Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Increase test coverage Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

Generic web analytics tracking implementation

605f980

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke requested review from everett980, rubenvp8510 and tiffon as code owners January 22, 2021 11:10

yurishkuro reviewed Jan 22, 2021

View reviewed changes

th3M1ke force-pushed the Generic-analytics-tracking-impl branch from 5b22d04 to 605f980 Compare February 1, 2021 23:57

Update due to comments, refactor Google Analytic tracker

3f71ca3

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke force-pushed the Generic-analytics-tracking-impl branch from 62f6f6a to 3f71ca3 Compare February 11, 2021 19:25

Add unit tests

6e147f7

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

Update tests

85fec84

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro closed this Feb 15, 2021

yurishkuro reopened this Feb 15, 2021

th3M1ke added 2 commits February 15, 2021 22:44

Update tests

171ca8f

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

Increase test coverage

186e6c8

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro merged commit abdf99d into jaegertracing:master Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic web analytics tracking implementation #681

Generic web analytics tracking implementation #681

th3M1ke commented Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

th3M1ke Jan 25, 2021 •

edited

Loading

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

th3M1ke Jan 25, 2021

yurishkuro Jan 26, 2021

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

yurishkuro Jan 22, 2021

th3M1ke commented Jan 25, 2021

yurishkuro commented Jan 26, 2021

th3M1ke commented Feb 11, 2021

yurishkuro commented Feb 11, 2021

th3M1ke commented Feb 11, 2021

th3M1ke commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 16, 2021

		@@ -0,0 +1,30 @@
		// Copyright (c) 2021 Uber Technologies, Inc.

	// Copyright (c) 2021 Uber Technologies, Inc.
	// Copyright (c) 2021 The Jaeger Authors.

		@@ -0,0 +1,187 @@
		// Copyright (c) 2021 Uber Technologies, Inc.

Generic web analytics tracking implementation #681

Generic web analytics tracking implementation #681

Conversation

th3M1ke commented Jan 22, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3M1ke Jan 25, 2021 • 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

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

th3M1ke commented Jan 25, 2021

yurishkuro commented Jan 26, 2021

th3M1ke commented Feb 11, 2021

yurishkuro commented Feb 11, 2021

th3M1ke commented Feb 11, 2021

th3M1ke commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 15, 2021

yurishkuro commented Feb 16, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

th3M1ke Jan 25, 2021 •

edited

Loading