Switch performance event traffic to transport endpoint #2593

zijianjoy · 2020-02-03T22:08:34Z

Implement API call to transport endpoint for dispatching Performance Monitoring events.
- Honor nextRequestWaitMillis from transport response.
- Act on responseAction for logResponseDetail: Push batch message back to queue if RETRY_REQUEST_LATER.
Fetch Remote Config to dynamically update transport endpoint Key.
Fetch Remote Config to get rollout percentage to transport:
1. If there is no template from Remote Config fetch, send events to cc.
2. If there is template but no rollout flag, send events to transport.
3. If there is rollout flag, generate hash value from instanceID:
  1. If hash < rollout percentage, send to transport.
  2. If hash >= rollout percentage, send to cc.

…esponse.

…onfigurable.

packages/performance/src/services/perf_logger.ts

packages/performance/src/services/remote_config_service.test.ts

packages/performance/src/services/transport_service.ts

packages/performance/src/utils/string_merger.ts

packages/performance/src/services/remote_config_service.ts

alikn · 2020-04-04T01:59:46Z

packages/performance/src/services/remote_config_service.ts

+    state === 'INSTANCE_STATE_UNSPECIFIED' ||
+    state === 'NO_TEMPLATE'
+  ) {
+    if (NO_TEMPLATE_CONFIGS.shouldSendToTransport !== undefined) {


Since there is no risk of shouldSendToTransport being one of the js falsey values, we can drop the "!== undefined" in the checks.

NO_TEMPLATE_CONFIGS.shouldSendToTransport can be false as boolean value. If changing the check above into if (NO_TEMPLATE_CONFIGS.shouldSendToTransport), then this check will return false if shouldSendToTransport is false. It will cause boolean value assignment inside the if block being skipped. Please correct me if I was wrong understanding the comment. Thank you!

packages/performance/src/services/remote_config_service.ts

packages/performance/src/services/remote_config_service.test.ts

alikn · 2020-04-06T02:14:01Z

packages/performance/src/services/transport_service.ts

 interface BatchEvent {
  message: string;
  eventTime: number;
 }

 /* eslint-disable camelcase */
-// CC accepted log format.
-interface CcBatchLogFormat {
+// CC/Transport accepted log format.


Rather than "transport" let's call it Firelog to make it clear what it is.

Happy to continue discussion on naming convention.

+1 on changing it to Firelog.

Transport is a mechanism and Clearcut and Firelog are the different types of it IMO.

Note: If you plan to change it please update at other places as well to have the distinction between Transport and Firelog clear.

However, if you want to have it Transport for abstraction reason where other parts of the SDK doesn't need to care about what's the underlying technology is (Clearcut, Firelog etc) than I am OK with that with 2 caveats:

To external world (other parts of Web SDK) the API would be sendEventsToTransport() (or something along the lines)

Internally (within this class) we need a way to know which transport mechanism are we using. I see the internal APIs doesn't talk about Firelog at all.

After discussion, I have changed the name of Transport to FL to avoid confusion. Please take a look and see whether it makes sense to you. Thank you both!

packages/performance/src/services/transport_service.ts

google-oss-bot · 2020-04-06T08:30:33Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`6898eb8`)	Head (`7074d74`)	Diff
@firebase/logger	module	?	4778.00	? (?)
	esm2017	?	3223.00	? (?)
	main	?	5087.00	? (?)
@firebase/storage	module	?	62706.00	? (?)
	esm2017	?	56574.00	? (?)
	main	?	62912.00	? (?)
@firebase/firestore/memory	browser	?	198105.00	? (?)
	module	?	196503.00	? (?)
	esm2017	?	155605.00	? (?)
	main	?	374451.00	? (?)
@firebase/util	browser	?	19454.00	? (?)
	module	?	18551.00	? (?)
	esm2017	?	17369.00	? (?)
	main	?	19481.00	? (?)
@firebase/app	lite-esm2017	?	7748.00	? (?)
	react-native	?	9862.00	? (?)
	browser	?	11108.00	? (?)
	module	?	11011.00	? (?)
	esm2017	?	9461.00	? (?)
	2 more entries not displayed here ...
@firebase/firestore	browser	?	252362.00	? (?)
	module	?	250342.00	? (?)
	esm2017	?	199040.00	? (?)
	main	?	486139.00	? (?)
@firebase/installations	module	?	21544.00	? (?)
	esm2017	?	16530.00	? (?)
	main	?	21995.00	? (?)
@firebase/functions	browser	?	9174.00	? (?)
	module	?	8994.00	? (?)
	esm2017	?	7010.00	? (?)
	main	?	9209.00	? (?)
@firebase/webchannel-wrapper	module	?	41226.00	? (?)
@firebase/webchannel-wrapper	main	?	40383.00	? (?)
@firebase/performance	browser	?	28112.00	? (?)
	module	?	27862.00	? (?)
	esm2017	?	26090.00	? (?)
	main	?	28112.00	? (?)
firebase	firebase-installations.js	?	19157.00	? (?)
	firebase-storage.js	?	40971.00	? (?)
	firebase-messaging.js	?	39120.00	? (?)
	firebase-auth.js	?	173406.00	? (?)
	firebase.js	?	827721.00	? (?)
	10 more entries not displayed here ...
@firebase/analytics	module	?	9370.00	? (?)
	esm2017	?	8681.00	? (?)
	main	?	9688.00	? (?)
@firebase/auth	module	?	177089.00	? (?)
@firebase/auth	main	?	177099.00	? (?)
@firebase/remote-config	browser	?	23103.00	? (?)
	module	?	22720.00	? (?)
	esm2017	?	17705.00	? (?)
	main	?	23103.00	? (?)
@firebase/testing	main	?	6607.00	? (?)
@firebase/messaging	module	?	30886.00	? (?)
	esm2017	?	23151.00	? (?)
	main	?	31343.00	? (?)
@firebase/database	browser	?	267892.00	? (?)
	module	?	266349.00	? (?)
	esm2017	?	234647.00	? (?)
	main	?	268656.00	? (?)
@firebase/component	browser	?	5164.00	? (?)
	module	?	5046.00	? (?)
	esm2017	?	3898.00	? (?)
	main	?	5164.00	? (?)
@firebase/polyfill	module	?	666.00	? (?)
@firebase/polyfill	main	?	733.00	? (?)

Metric Unit: byte

Test Logs

https://github.com/firebase/firebase-js-sdk/actions/runs/73756304

packages/performance/CHANGELOG.md

ramanpreetSinghKhinda

Still reviewing...

packages/performance/CHANGELOG.md

ramanpreetSinghKhinda · 2020-04-06T18:00:02Z

packages/performance/src/services/perf_logger.test.ts

@@ -301,5 +302,78 @@ describe('Performance Monitoring > perf_logger', () => {
        EXPECTED_NETWORK_MESSAGE
      );
    });
+
+    it('skips performance collection if domain is cc', () => {


Why we want to skip the collection in this case?

Fireperf automatically collects network request (including request to cc). However, to avoid infinite loop of network request collection (request to CC -> request is captured as performance resource -> another request to CC), if the domain is cc, then we will not collect the network request.

Thanks James. Based on our offline discussion this is validating that we don't instrument Network Request to CC/FL sent by our own SDK.

Since it took a while to understand this scenario it would be good to add some documentation to make it clear.

Thank you Raman! Added comments to explain that we don't instrument requests sent from SDK itself in both production code and test code.

packages/performance/src/services/perf_logger.test.ts

ramanpreetSinghKhinda · 2020-04-06T18:43:46Z

packages/performance/src/services/settings_service.ts

+    'tp:/ieaeogn-agolai.o/1frlglgc/o'
+  );
+
+  transportKey = mergeStrings('AzSC8r6ReiGqFMyfvgow', 'Iayx0u-XT3vksVM-pIV');


Since Web SDK is an Open Source project, I am wondering how difficult it would be for any intruder to crack our API?

Do we want to get a review from Firelog team to see if it's OK to expose this API Key even if it's juggled (considering the encoding logic is also public, so anyone can decode it very easily)?

Storing plain text key on GitHub will automatically trigger alert to the owner, therefore we cannot store plain text key directly. Current approach is to align with mobile SDK change for interleave encoding. We have previously confirmed with the team for this approach.

ramanpreetSinghKhinda · 2020-04-06T18:48:19Z

packages/performance/src/services/transport_service.ts

 interface BatchEvent {
  message: string;
  eventTime: number;
 }

 /* eslint-disable camelcase */
-// CC accepted log format.
-interface CcBatchLogFormat {
+// CC/Transport accepted log format.


+1 on changing it to Firelog.

Transport is a mechanism and Clearcut and Firelog are the different types of it IMO.

Note: If you plan to change it please update at other places as well to have the distinction between Transport and Firelog clear.

However, if you want to have it Transport for abstraction reason where other parts of the SDK doesn't need to care about what's the underlying technology is (Clearcut, Firelog etc) than I am OK with that with 2 caveats:

To external world (other parts of Web SDK) the API would be sendEventsToTransport() (or something along the lines)

Internally (within this class) we need a way to know which transport mechanism are we using. I see the internal APIs doesn't talk about Firelog at all.

ramanpreetSinghKhinda · 2020-04-06T18:54:43Z

packages/performance/src/services/transport_service.ts

+): Promise<void> {
+  // Gradually rollout traffic from cc to transport using remote config.
+  if (SettingsService.getInstance().shouldSendToTransport) {
+    return sendEventsToTransport(data, staged);


FYI: https://github.com/firebase/firebase-js-sdk/pull/2593/files#r403797030

One example is this.

packages/performance/src/services/perf_logger.test.ts

packages/performance/src/services/perf_logger.ts

packages/performance/src/services/perf_logger.test.ts

packages/performance/src/services/remote_config_service.test.ts

packages/performance/src/services/remote_config_service.ts

visumickey · 2020-04-06T23:58:49Z

packages/performance/src/services/remote_config_service.ts

+function getHashPercent(seed: string): number {
+  let hash = 0;
+  for (let i = 0; i < seed.length; i++) {
+    hash = (hash << 3) + hash - seed.charCodeAt(i);


Any reference to where we chose to pick this logic? I think good to callout the source for this one.

Added comment to call out the source.

Am I missing something? I still don't see the reference link to the algorithm.

This is at the function comment: Implementation from String.hashCode() in Java.. This will keep the comment concise and we can find reference online.

visumickey · 2020-04-06T23:59:42Z

packages/performance/src/services/remote_config_service.ts

+  if (iid.length === 0) {
+    return false;
+  }
+  return getHashPercent(iid) < rolloutPercent;


Should we check for the rolloutPercent ranges?

SDK will honor the percentage because we don't perform special handling if it is out of valid range, backend can inspect and control flag value if this is not correct.

Sure, even if BE does that, having a range check would make the SDK cleaner.

Currently we don't have range check for all RC fetch result. We can add them but I don't expect SDK to handle this differently if the value is out-of-range, therefore not adding additional check for now and revisit it in the future.

packages/performance/src/services/remote_config_service.test.ts

packages/performance/src/services/remote_config_service.ts

packages/performance/src/services/transport_service.ts

visumickey · 2020-04-07T01:30:33Z

packages/performance/src/utils/errors.ts

-    'Custom metric name {$customMetricName} is invalid'
+    'Custom metric name {$customMetricName} is invalid',
+  [ErrorCode.INVALID_STRING_MERGER_PARAMETER]:
+    'Input for String merger is invalid'


This is quite a cryptic message for the developers. Can we make this message a bit more contextual? Eg. Unable to form API Key/URL?

This message is not planned for developers to resolve by themselves, it is useful to identify the issue quickly when developer takes this message to Firebase customer support.

Sure, but still the developer looking at that message would be confused as to how to act on that message.

Added more information to let developer know about the next step.

Feiyang1

Overall it looks good to me except for the URL API usage.
Will we eventually replace cc with fl? The perf SDK is very sensitive to size and this PR increases it (It'd be interesting to see the actual increase in percentage - Currently the size check is broken for this PR, but I will fix it so we can get the number).

Feiyang1 · 2020-04-07T20:42:46Z

packages/performance/src/services/perf_logger.ts

+
+  // Do not log the js sdk's call to transport service domain to avoid unnecessary cycle.
+  // Need to blacklist both old and new endpoints to avoid migration gap.
+  const networkRequestHostName = new URL(networkRequest.url).hostname;


URL API is not available in IE11. Are you able to just use string parsing? I'm reluctant to ask people to add an additional polyfill unless there is a strong reason.

Thank you for the heads-up! Decided not to use URL API and match the full URL without search parameter for cc and fl endpoints.

Feiyang1 · 2020-04-07T20:55:34Z

packages/performance/src/services/remote_config_service.ts

+/**
+ * True if event should be sent to Fl transport endpoint rather than CC transport endpoint.
+ * rolloutPercent is in range [0.0, 100.0].
+ * @param {string} iid Installation ID which identifies a web app installed on client.


no need to specify the parameter type in Typescript

Removed parameter type from comments.

zijianjoy · 2020-04-08T01:26:03Z

Overall it looks good to me except for the URL API usage.
Will we eventually replace cc with fl? The perf SDK is very sensitive to size and this PR increases it (It'd be interesting to see the actual increase in percentage - Currently the size check is broken for this PR, but I will fix it so we can get the number).

Thank you Fei! I have removed the URL API usage. We are expecting to remove cc usage completely after 2 releases. Expecting the SDK size to decrease by then.

Feiyang1 · 2020-04-08T16:48:09Z

packages/performance/src/services/remote_config_service.ts

@@ -271,9 +271,6 @@ function shouldLogAfterSampling(samplingRate: number): boolean {
 /**
 * True if event should be sent to Fl transport endpoint rather than CC transport endpoint.
 * rolloutPercent is in range [0.0, 100.0].
- * @param {string} iid Installation ID which identifies a web app installed on client.


hm, actually I suggested to only remove the type information in the comment, that is e.g. {string}, and we should keep the rest of the comments.

Thanks for the callout! I overlooked your message about parameter type instead of parameter.

zijianjoy added 5 commits February 1, 2020 21:55

Implement traffic switch logic between cc and transport.

591c769

Implement transport traffic rollout logic and handling of transport r…

e86086a

…esponse.

Revert minor version bump and yarn.lock changes

3a704dd

Add logic to retrieve transport endpoint from RemoteConfig Fetch

3d5342a

Make Transport Key configurable only, transport endpoint URL is not c…

8d505a7

…onfigurable.

zijianjoy requested a review from alikn March 12, 2020 21:34

zijianjoy marked this pull request as ready for review March 12, 2020 21:34

alikn reviewed Mar 16, 2020

View reviewed changes

zijianjoy requested a review from visumickey March 17, 2020 16:42

Address comments: remove request headers, update tests.

cdc8501

zijianjoy requested a review from ramanpreetSinghKhinda March 31, 2020 23:04

alikn reviewed Apr 6, 2020

View reviewed changes

Address comments on testing for rollout flag and empty state handling.

26ea86b

visumickey reviewed Apr 6, 2020

View reviewed changes

packages/performance/CHANGELOG.md Outdated Show resolved Hide resolved

Update release note of transport service change.

ba78d97

ramanpreetSinghKhinda reviewed Apr 6, 2020

View reviewed changes

Change naming from transport to Fl.

4f025e4

alikn approved these changes Apr 7, 2020

View reviewed changes

visumickey reviewed Apr 7, 2020

View reviewed changes

ramanpreetSinghKhinda approved these changes Apr 7, 2020

View reviewed changes

packages/performance/src/services/remote_config_service.test.ts Show resolved Hide resolved

packages/performance/src/services/remote_config_service.ts Show resolved Hide resolved

packages/performance/src/services/remote_config_service.ts Outdated Show resolved Hide resolved

visumickey reviewed Apr 7, 2020

View reviewed changes

Added comment, variable naming changes.

b787386

zijianjoy requested a review from Feiyang1 April 7, 2020 16:53

Feiyang1 suggested changes Apr 7, 2020

View reviewed changes

zijianjoy added 2 commits April 7, 2020 17:47

Update function comment and replace URL usage with regex.

8b8c321

Use full URL instead of extracting domain name.

4180537

visumickey approved these changes Apr 8, 2020

View reviewed changes

Feiyang1 reviewed Apr 8, 2020

View reviewed changes

Bring back function comment but not the parameter type.

0e69357

Feiyang1 approved these changes Apr 8, 2020

View reviewed changes

Update error code description with next step

eb1bcba

zijianjoy merged commit d689b75 into master Apr 8, 2020

firebase locked and limited conversation to collaborators May 9, 2020

Switch performance event traffic to transport endpoint #2593

Switch performance event traffic to transport endpoint #2593

Uh oh!

Conversation

zijianjoy commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

google-oss-bot commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

Uh oh!

Uh oh!

ramanpreetSinghKhinda left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

zijianjoy commented Feb 3, 2020 •

edited

Loading

google-oss-bot commented Apr 6, 2020 •

edited

Loading

zijianjoy Apr 8, 2020 •

edited

Loading