Skip to content

Switch performance event traffic to transport endpoint #2593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 8, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/performance/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
# Unreleased
- [changed] Update the transport mechanism of Fireperf events.

# 0.2.30
- [changed] Internal transport protocol update from proto2 to proto3.
78 changes: 78 additions & 0 deletions packages/performance/src/services/perf_logger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { SDK_VERSION } from '../constants';
import * as attributeUtils from '../utils/attributes_utils';
import { createNetworkRequestEntry } from '../resources/network_request';
import '../../test/setup';
import { mergeStrings } from '../utils/string_merger';

describe('Performance Monitoring > perf_logger', () => {
const IID = 'idasdfsffe';
Expand Down Expand Up @@ -301,5 +302,82 @@ describe('Performance Monitoring > perf_logger', () => {
EXPECTED_NETWORK_MESSAGE
);
});

// Performance SDK doesn't instrument requests sent from SDK itself, therefore blacklist
// requests sent to cc endpoint.
it('skips performance collection if domain is cc', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we want to skip the collection in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const CC_NETWORK_PERFORMANCE_ENTRY: PerformanceResourceTiming = {
connectEnd: 0,
connectStart: 0,
decodedBodySize: 0,
domainLookupEnd: 0,
domainLookupStart: 0,
duration: 39.610000094398856,
encodedBodySize: 0,
entryType: 'resource',
fetchStart: 5645.689999917522,
initiatorType: 'fetch',
name: 'https://firebaselogging.googleapis.com/v0cc/log?message=a',
nextHopProtocol: 'http/2+quic/43',
redirectEnd: 0,
redirectStart: 0,
requestStart: 0,
responseEnd: 5685.300000011921,
responseStart: 0,
secureConnectionStart: 0,
startTime: 5645.689999917522,
transferSize: 0,
workerStart: 0,
toJSON: () => {}
};
getIidStub.returns(IID);
SettingsService.getInstance().loggingEnabled = true;
SettingsService.getInstance().logNetworkAfterSampling = true;
// Calls logNetworkRequest under the hood.
createNetworkRequestEntry(CC_NETWORK_PERFORMANCE_ENTRY);
clock.tick(1);

expect(addToQueueStub).not.called;
});

// Performance SDK doesn't instrument requests sent from SDK itself, therefore blacklist
// requests sent to fl endpoint.
it('skips performance collection if domain is fl', () => {
const FL_NETWORK_PERFORMANCE_ENTRY: PerformanceResourceTiming = {
connectEnd: 0,
connectStart: 0,
decodedBodySize: 0,
domainLookupEnd: 0,
domainLookupStart: 0,
duration: 39.610000094398856,
encodedBodySize: 0,
entryType: 'resource',
fetchStart: 5645.689999917522,
initiatorType: 'fetch',
name: mergeStrings(
'hts/frbslgigp.ogepscmv/ieo/eaylg',
'tp:/ieaeogn-agolai.o/1frlglgc/o'
),
nextHopProtocol: 'http/2+quic/43',
redirectEnd: 0,
redirectStart: 0,
requestStart: 0,
responseEnd: 5685.300000011921,
responseStart: 0,
secureConnectionStart: 0,
startTime: 5645.689999917522,
transferSize: 0,
workerStart: 0,
toJSON: () => {}
};
getIidStub.returns(IID);
SettingsService.getInstance().loggingEnabled = true;
SettingsService.getInstance().logNetworkAfterSampling = true;
// Calls logNetworkRequest under the hood.
createNetworkRequestEntry(FL_NETWORK_PERFORMANCE_ENTRY);
clock.tick(1);

expect(addToQueueStub).not.called;
});
});
});
16 changes: 14 additions & 2 deletions packages/performance/src/services/perf_logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,19 @@ export function logNetworkRequest(networkRequest: NetworkRequest): void {
if (!settingsService.instrumentationEnabled) {
return;
}
// Do not log the js sdk's call to cc service to avoid unnecessary cycle.
if (networkRequest.url === settingsService.logEndPointUrl.split('?')[0]) {

// 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 networkRequestUrl = networkRequest.url;

// Blacklist old log endpoint and new transport endpoint.
// Because Performance SDK doesn't instrument requests sent from SDK itself.
const logEndpointUrl = settingsService.logEndPointUrl.split('?')[0];
const flEndpointUrl = settingsService.flTransportEndpointUrl.split('?')[0];
if (
networkRequestUrl === logEndpointUrl ||
networkRequestUrl === flEndpointUrl
) {
return;
}

Expand All @@ -165,6 +176,7 @@ export function logNetworkRequest(networkRequest: NetworkRequest): void {
setTimeout(() => sendLog(networkRequest, ResourceType.NetworkRequest), 0);
}


function serializer(
resource: NetworkRequest | Trace,
resourceType: ResourceType
Expand Down
157 changes: 148 additions & 9 deletions packages/performance/src/services/remote_config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,30 @@ import { SettingsService } from './settings_service';
import { CONFIG_EXPIRY_LOCAL_STORAGE_KEY } from '../constants';
import { setupApi, Api } from './api_service';
import * as iidService from './iid_service';
import { getConfig } from './remote_config_service';
import { getConfig, isDestFl } from './remote_config_service';
import { FirebaseApp } from '@firebase/app-types';
import '../../test/setup';

describe('Performance Monitoring > remote_config_service', () => {
const IID = 'asd123';
const AUTH_TOKEN = 'auth_token';
const LOG_URL = 'https://firebaselogging.test.com';
const TRANSPORT_KEY = 'pseudo-transport-key';
const LOG_SOURCE = 2;
const NETWORK_SAMPLIG_RATE = 0.25;
const TRACE_SAMPLING_RATE = 0.5;
const GLOBAL_CLOCK_NOW = 1556524895326;
const STRINGIFIED_CONFIG = `{"entries":{"fpr_enabled":"true",\
"fpr_log_endpoint_url":"https://firebaselogging.test.com",\
"fpr_log_source":"2","fpr_vc_network_request_sampling_rate":"0.250000",\
"fpr_vc_session_sampling_rate":"0.250000","fpr_vc_trace_sampling_rate":"0.500000"},\
"state":"UPDATE"}`;
"fpr_log_endpoint_url":"https://firebaselogging.test.com",\
"fpr_log_transport_key":"pseudo-transport-key",\
"fpr_log_source":"2","fpr_vc_network_request_sampling_rate":"0.250000",\
"fpr_log_transport_web_percent":"100.0",\
"fpr_vc_session_sampling_rate":"0.250000","fpr_vc_trace_sampling_rate":"0.500000"},\
"state":"UPDATE"}`;
const PROJECT_ID = 'project1';
const APP_ID = '1:23r:web:fewq';
const API_KEY = 'asdfghjk';
const NOT_VALID_CONFIG = 'not a valid config and should not be used';

let clock: SinonFakeTimers;

Expand Down Expand Up @@ -127,7 +131,11 @@ describe('Performance Monitoring > remote_config_service', () => {
expect(getItemStub).to.be.called;
expect(SettingsService.getInstance().loggingEnabled).to.be.true;
expect(SettingsService.getInstance().logEndPointUrl).to.equal(LOG_URL);
expect(SettingsService.getInstance().transportKey).to.equal(
TRANSPORT_KEY
);
expect(SettingsService.getInstance().logSource).to.equal(LOG_SOURCE);
expect(SettingsService.getInstance().shouldSendToFl).to.be.true;
expect(
SettingsService.getInstance().networkRequestsSamplingRate
).to.equal(NETWORK_SAMPLIG_RATE);
Expand Down Expand Up @@ -164,7 +172,11 @@ describe('Performance Monitoring > remote_config_service', () => {
expect(getItemStub).to.be.calledOnce;
expect(SettingsService.getInstance().loggingEnabled).to.be.true;
expect(SettingsService.getInstance().logEndPointUrl).to.equal(LOG_URL);
expect(SettingsService.getInstance().transportKey).to.equal(
TRANSPORT_KEY
);
expect(SettingsService.getInstance().logSource).to.equal(LOG_SOURCE);
expect(SettingsService.getInstance().shouldSendToFl).to.be.true;
expect(
SettingsService.getInstance().networkRequestsSamplingRate
).to.equal(NETWORK_SAMPLIG_RATE);
Expand All @@ -180,7 +192,7 @@ describe('Performance Monitoring > remote_config_service', () => {
setup(
{
expiry: EXPIRY_LOCAL_STORAGE_VALUE,
config: 'not a valid config and should not be used'
config: NOT_VALID_CONFIG
},
{ reject: true }
);
Expand All @@ -201,7 +213,7 @@ describe('Performance Monitoring > remote_config_service', () => {
setup(
{
expiry: EXPIRY_LOCAL_STORAGE_VALUE,
config: 'not a valid config and should not be used'
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response(STRINGIFIED_PARTIAL_CONFIG) }
);
Expand All @@ -214,18 +226,145 @@ describe('Performance Monitoring > remote_config_service', () => {
it('uses secondary configs if the response does not have any fields', async () => {
// Expired local config.
const EXPIRY_LOCAL_STORAGE_VALUE = '1556524895320';
const STRINGIFIED_PARTIAL_CONFIG = '{"state":"NO TEMPLATE"}';
const STRINGIFIED_PARTIAL_CONFIG = '{"state":"NO_TEMPLATE"}';

setup(
{
expiry: EXPIRY_LOCAL_STORAGE_VALUE,
config: 'not a valid config and should not be used'
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response(STRINGIFIED_PARTIAL_CONFIG) }
);
await getConfig(IID);

expect(SettingsService.getInstance().loggingEnabled).to.be.true;
});

it('marks event destination to cc if there is no template', async () => {
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response('{"state":"NO_TEMPLATE"}') }
);
await getConfig(IID);

// If no template, will send to cc.
expect(SettingsService.getInstance().shouldSendToFl).to.be.false;
});

it('marks event destination to cc if instance state unspecified', async () => {
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{
reject: false,
value: new Response('{"state":"INSTANCE_STATE_UNSPECIFIED"}')
}
);
await getConfig(IID);

// If instance state unspecified, will send to cc.
expect(SettingsService.getInstance().shouldSendToFl).to.be.false;
});

it("marks event destination to cc if state doesn't exist", async () => {
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response('{}') }
);
await getConfig(IID);

// If "state" doesn't exist, will send to cc.
expect(SettingsService.getInstance().shouldSendToFl).to.be.false;
});

it('marks event destination to Fl if template exists but no rollout flag', async () => {
const CONFIG_WITHOUT_ROLLOUT_FLAG = `{"entries":{"fpr_enabled":"true",\
"fpr_log_endpoint_url":"https://firebaselogging.test.com",\
"fpr_log_source":"2","fpr_vc_network_request_sampling_rate":"0.250000",\
"fpr_vc_session_sampling_rate":"0.250000","fpr_vc_trace_sampling_rate":"0.500000"},\
"state":"UPDATE"}`;
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response(CONFIG_WITHOUT_ROLLOUT_FLAG) }
);
await getConfig(IID);

// If template exists but no rollout flag, will send to Fl.
expect(SettingsService.getInstance().shouldSendToFl).to.be.true;
});

it('marks event destination to cc when instance is outside of rollout range', async () => {
const CONFIG_WITH_ROLLOUT_FLAG_10 = `{"entries":{"fpr_enabled":"true",\
"fpr_log_endpoint_url":"https://firebaselogging.test.com",\
"fpr_log_source":"2","fpr_vc_network_request_sampling_rate":"0.250000",\
"fpr_log_transport_web_percent":"10.0",\
"fpr_vc_session_sampling_rate":"0.250000","fpr_vc_trace_sampling_rate":"0.500000"},\
"state":"UPDATE"}`;
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response(CONFIG_WITH_ROLLOUT_FLAG_10) }
);
await getConfig(IID);

// If rollout flag exists, will send to cc when this instance is out of rollout scope.
expect(SettingsService.getInstance().shouldSendToFl).to.be.false;
});

it('marks event destination to Fl when instance is within rollout range', async () => {
const CONFIG_WITH_ROLLOUT_FLAG_40 = `{"entries":{"fpr_enabled":"true",\
"fpr_log_endpoint_url":"https://firebaselogging.test.com",\
"fpr_log_source":"2","fpr_vc_network_request_sampling_rate":"0.250000",\
"fpr_log_transport_web_percent":"40.0",\
"fpr_vc_session_sampling_rate":"0.250000","fpr_vc_trace_sampling_rate":"0.500000"},\
"state":"UPDATE"}`;
setup(
{
// Expired local config.
expiry: '1556524895320',
config: NOT_VALID_CONFIG
},
{ reject: false, value: new Response(CONFIG_WITH_ROLLOUT_FLAG_40) }
);
await getConfig(IID);

// If rollout flag exists, will send to Fl when this instance is within rollout scope.
expect(SettingsService.getInstance().shouldSendToFl).to.be.true;
});
});

describe('isDestFl', () => {
it('marks traffic to cc when rollout percentage is 0', () => {
const shouldSendToFl = isDestFl('abc', 0); // Hash percentage of "abc" is 38%.
expect(shouldSendToFl).to.be.false;
});

it('marks traffic to Fl when rollout percentage is 100', () => {
const shouldSendToFl = isDestFl('abc', 100); // Hash percentage of "abc" is 38%.
expect(shouldSendToFl).to.be.true;
});

it('marks traffic to Fl if hash percentage is lower than rollout percentage 50%', () => {
const shouldSendToFl = isDestFl('abc', 50); // Hash percentage of "abc" is 38%.
expect(shouldSendToFl).to.be.true;
});
});
});
Loading