From 640708c2e27c01010d6e6f27f24fa144e3355114 Mon Sep 17 00:00:00 2001 From: Mend Renovate Date: Wed, 30 Nov 2022 15:42:14 +0100 Subject: [PATCH 1/6] chore(deps): update dependency @types/uuid to v9 (#1114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@types/uuid](https://togithub.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/uuid) ([source](https://togithub.com/DefinitelyTyped/DefinitelyTyped)) | [`^8.0.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fuuid/8.3.4/9.0.0) | [![age](https://badges.renovateapi.com/packages/npm/@types%2fuuid/9.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/@types%2fuuid/9.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/@types%2fuuid/9.0.0/compatibility-slim/8.3.4)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/@types%2fuuid/9.0.0/confidence-slim/8.3.4)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/cloud-debug-nodejs). --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8d8b356c..ed2bd2d5 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "@types/proxyquire": "^1.3.28", "@types/semver": "^7.0.0", "@types/tmp": "^0.2.0", - "@types/uuid": "^8.0.0", + "@types/uuid": "^9.0.0", "cpy-cli": "^4.0.0", "cross-env": "^7.0.0", "execa": "^5.0.0", From a188e347131168d21d01f77267863e53ae1043a8 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Fri, 2 Dec 2022 18:32:13 -0500 Subject: [PATCH 2/6] feat: add error handling to firebase controller. (#1116) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/cloud-debug-nodejs/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary) Fixes #1115 🦕 --- src/agent/firebase-controller.ts | 173 +++++++++++++++++++++---------- test/test-firebase-controller.ts | 166 ++++++++++++++++++++++++++++- 2 files changed, 285 insertions(+), 54 deletions(-) diff --git a/src/agent/firebase-controller.ts b/src/agent/firebase-controller.ts index 3f02066c..ade04399 100644 --- a/src/agent/firebase-controller.ts +++ b/src/agent/firebase-controller.ts @@ -29,6 +29,8 @@ import * as gcpMetadata from 'gcp-metadata'; import * as util from 'util'; const debuglog = util.debuglog('cdbg.firebase'); +const FIREBASE_APP_NAME = 'cdbg'; + export class FirebaseController implements Controller { db: firebase.database.Database; debuggeeId?: string; @@ -74,26 +76,49 @@ export class FirebaseController implements Controller { if (options.databaseUrl) { databaseUrl = options.databaseUrl; } else { - // TODO: Test whether this exists. If not, fall back to -default. + // TODO: Add fallback to -default databaseUrl = `https://${projectId}-cdbg.firebaseio.com`; } + let app: firebase.app.App; if (credential) { - firebase.initializeApp({ - credential: credential, - databaseURL: databaseUrl, - }); + app = firebase.initializeApp( + { + credential: credential, + databaseURL: databaseUrl, + }, + FIREBASE_APP_NAME + ); } else { // Use the default credentials. - firebase.initializeApp({ - databaseURL: databaseUrl, - }); + app = firebase.initializeApp( + { + databaseURL: databaseUrl, + }, + 'cdbg' + ); } const db = firebase.database(); - // TODO: Test this setup and emit a reasonable error. - debuglog('Firebase app initialized. Connected to', databaseUrl); + // Test the connection by reading the schema version. + try { + const version_snapshot = await db.ref('cdbg/schema_version').get(); + if (version_snapshot) { + const version = version_snapshot.val(); + debuglog( + `Firebase app initialized. Connected to ${databaseUrl}` + + ` with schema version ${version}` + ); + } else { + app.delete(); + throw new Error('failed to fetch schema version from database'); + } + } catch (e) { + app.delete(); + throw e; + } + return db; } @@ -135,6 +160,7 @@ export class FirebaseController implements Controller { // This MUST be consistent across all debuggee instances. // TODO: JSON.stringify may provide different strings if labels are added // in different orders. + debuggee.id = ''; // Don't use the debuggee id when computing the id. const debuggeeHash = crypto .createHash('sha1') .update(JSON.stringify(debuggee)) @@ -143,11 +169,14 @@ export class FirebaseController implements Controller { debuggee.id = this.debuggeeId; const debuggeeRef = this.db.ref(`cdbg/debuggees/${this.debuggeeId}`); - debuggeeRef.set(debuggee); - - // TODO: Handle errors. I can .set(data, (error) => if (error) {}) - const agentId = 'unsupported'; - callback(null, {debuggee, agentId}); + debuggeeRef.set(debuggee, err => { + if (err) { + callback(err); + } else { + const agentId = 'unsupported'; + callback(null, {debuggee, agentId}); + } + }); } /** @@ -156,11 +185,11 @@ export class FirebaseController implements Controller { * @param {!Breakpoint} breakpoint * @param {!Function} callback accepting (err, body) */ - updateBreakpoint( + async updateBreakpoint( debuggee: Debuggee, breakpoint: stackdriver.Breakpoint, callback: (err?: Error, body?: {}) => void - ): void { + ): Promise { debuglog('updating a breakpoint'); assert(debuggee.id, 'should have a registered debuggee'); @@ -184,32 +213,43 @@ export class FirebaseController implements Controller { // https://firebase.google.com/docs/reference/rest/database#section-server-values breakpoint_map['finalTimeUnixMsec'] = {'.sv': 'timestamp'}; - this.db - .ref(`cdbg/breakpoints/${this.debuggeeId}/active/${breakpoint.id}`) - .remove(); + try { + await this.db + .ref(`cdbg/breakpoints/${this.debuggeeId}/active/${breakpoint.id}`) + .remove(); + } catch (err) { + debuglog(`failed to delete breakpoint ${breakpoint.id}: ` + err); + callback(err as Error); + throw err; + } + + try { + if (is_snapshot) { + // We could also restrict this to only write to this node if it wasn't + // an error and there is actual snapshot data. For now though we'll + // write it regardless, makes sense if you want to get everything for + // a snapshot it's at this location, regardless of what it contains. + await this.db + .ref(`cdbg/breakpoints/${this.debuggeeId}/snapshot/${breakpoint.id}`) + .set(breakpoint_map); + // Now strip the snapshot data for the write to 'final' path. + const fields_to_strip = [ + 'evaluatedExpressions', + 'stackFrames', + 'variableTable', + ]; + fields_to_strip.forEach(field => delete breakpoint_map[field]); + } - // TODO: error handling from here on - if (is_snapshot) { - // We could also restrict this to only write to this node if it wasn't - // an error and there is actual snapshot data. For now though we'll - // write it regardless, makes sense if you want to get everything for - // a snapshot it's at this location, regardless of what it contains. - this.db - .ref(`cdbg/breakpoints/${this.debuggeeId}/snapshot/${breakpoint.id}`) + await this.db + .ref(`cdbg/breakpoints/${this.debuggeeId}/final/${breakpoint.id}`) .set(breakpoint_map); - // Now strip the snapshot data for the write to 'final' path. - const fields_to_strip = [ - 'evaluatedExpressions', - 'stackFrames', - 'variableTable', - ]; - fields_to_strip.forEach(field => delete breakpoint_map[field]); + } catch (err) { + debuglog(`failed to finalize breakpoint ${breakpoint.id}: ` + err); + callback(err as Error); + throw err; } - this.db - .ref(`cdbg/breakpoints/${this.debuggeeId}/final/${breakpoint.id}`) - .set(breakpoint_map); - // Indicate success to the caller. callback(); } @@ -224,20 +264,42 @@ export class FirebaseController implements Controller { this.bpRef = this.db.ref(`cdbg/breakpoints/${this.debuggeeId}/active`); let breakpoints = [] as stackdriver.Breakpoint[]; - this.bpRef.on('child_added', (snapshot: firebase.database.DataSnapshot) => { - debuglog(`new breakpoint: ${snapshot.key}`); - const breakpoint = snapshot.val(); - breakpoint.id = snapshot.key; - breakpoints.push(breakpoint); - callback(null, breakpoints); - }); - this.bpRef.on('child_removed', snapshot => { - // remove the breakpoint. - const bpId = snapshot.key; - breakpoints = breakpoints.filter(bp => bp.id !== bpId); - debuglog(`breakpoint removed: ${bpId}`); - callback(null, breakpoints); - }); + this.bpRef.on( + 'child_added', + (snapshot: firebase.database.DataSnapshot) => { + debuglog(`new breakpoint: ${snapshot.key}`); + const breakpoint = snapshot.val(); + breakpoint.id = snapshot.key; + breakpoints.push(breakpoint); + callback(null, breakpoints); + }, + (e: Error) => { + debuglog( + 'unable to listen to child_added events on ' + + `cdbg/breakpoints/${this.debuggeeId}/active. ` + + 'Please check your database settings.' + ); + callback(e, []); + } + ); + this.bpRef.on( + 'child_removed', + snapshot => { + // remove the breakpoint. + const bpId = snapshot.key; + breakpoints = breakpoints.filter(bp => bp.id !== bpId); + debuglog(`breakpoint removed: ${bpId}`); + callback(null, breakpoints); + }, + (e: Error) => { + debuglog( + 'unable to listen to child_removed events on ' + + `cdbg/breakpoints/${this.debuggeeId}/active. ` + + 'Please check your database settings.' + ); + callback(e, []); + } + ); } stop(): void { @@ -245,5 +307,10 @@ export class FirebaseController implements Controller { this.bpRef.off(); this.bpRef = undefined; } + try { + firebase.app(FIREBASE_APP_NAME).delete(); + } catch (err) { + debuglog(`failed to tear down firebase app: ${err})`); + } } } diff --git a/test/test-firebase-controller.ts b/test/test-firebase-controller.ts index 1014345d..9b1d5ba5 100644 --- a/test/test-firebase-controller.ts +++ b/test/test-firebase-controller.ts @@ -44,6 +44,10 @@ class MockReference { // Simplification: there's only one listener for each event type. listeners = new Map any>(); + // Test options + shouldFail = false; + failMessage?: string; + constructor(key: string, parentRef?: MockReference) { this.key = key.slice(); this.parentRef = parentRef; @@ -90,6 +94,13 @@ class MockReference { } set(value: any, onComplete?: (a: Error | null) => any): Promise { + if (this.shouldFail) { + this.shouldFail = false; + if (onComplete) { + onComplete(new Error(this.failMessage)); + } + } + let creating = false; if (!this.value) { creating = true; @@ -121,6 +132,11 @@ class MockReference { off() { // No-op. Needed to cleanly detach in the real firebase implementation. } + + failNextSet(errorMessage: string) { + this.shouldFail = true; + this.failMessage = errorMessage; + } } /* eslint-enable @typescript-eslint/no-explicit-any */ @@ -152,7 +168,7 @@ describe('Firebase Controller', () => { const db = new MockDatabase(); // Debuggee Id is based on the sha1 hash of the json representation of // the debuggee. - const debuggeeId = 'd-b9dbb5e7'; + const debuggeeId = 'd-008335af'; const controller = new FirebaseController( db as {} as firebase.database.Database ); @@ -167,6 +183,20 @@ describe('Firebase Controller', () => { done(); }); }); + it('should error out gracefully', done => { + const db = new MockDatabase(); + // Debuggee Id is based on the sha1 hash of the json representation of + // the debuggee. + const debuggeeId = 'd-008335af'; + db.mockRef(`cdbg/debuggees/${debuggeeId}`).failNextSet('mocked failure'); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.register(debuggee, (err, result) => { + assert(err, 'expecting an error'); + done(); + }); + }); }); describe('subscribeToBreakpoints', () => { @@ -326,5 +356,139 @@ describe('Firebase Controller', () => { done(); }); }); + it('should throw an error if the delete fails', done => { + const breakpointId = 'breakpointId'; + const debuggeeId = 'debuggeeId'; + const breakpoint: stackdriver.Breakpoint = { + id: breakpointId, + action: 'CAPTURE', + location: {path: 'foo.js', line: 99}, + } as stackdriver.Breakpoint; + const debuggee: Debuggee = {id: 'fake-debuggee'} as Debuggee; + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.debuggeeId = debuggeeId; + + db.ref(`cdbg/breakpoints/${debuggeeId}/active`).on( + 'child_removed', + data => { + throw new Error('mock remove failure'); + } + ); + + let finalized = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/final`).on('child_added', data => { + finalized = true; + }); + + let snapshotted = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( + 'child_added', + data => { + snapshotted = true; + } + ); + controller.updateBreakpoint(debuggee as Debuggee, breakpoint, err => { + assert(err, 'expecting an error'); + assert(!finalized, 'should not have been finalized'); + assert(!snapshotted, 'should not have been snapshotted'); + done(); + }); + }); + it('throw an error if the finalization fails', done => { + const breakpointId = 'breakpointId'; + const debuggeeId = 'debuggeeId'; + const breakpoint: stackdriver.Breakpoint = { + id: breakpointId, + action: 'CAPTURE', + location: {path: 'foo.js', line: 99}, + } as stackdriver.Breakpoint; + const debuggee: Debuggee = {id: 'fake-debuggee'} as Debuggee; + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.debuggeeId = debuggeeId; + + let removed = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/active`).on( + 'child_removed', + data => { + assert.strictEqual(data.key, breakpointId); + removed = true; + } + ); + + db.ref(`cdbg/breakpoints/${debuggeeId}/final`).on('child_added', data => { + assert.strictEqual(data.key, breakpointId); + throw new Error('mock write failure'); + }); + + let snapshotted = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( + 'child_added', + data => { + snapshotted = true; + } + ); + + controller.updateBreakpoint(debuggee as Debuggee, breakpoint, err => { + assert(err, 'expecting an error'); + assert(removed, 'should have been removed'); + assert(snapshotted, 'should have been snapshotted'); + done(); + }); + }); + it('throw an error if writing the snapshot fails', done => { + const breakpointId = 'breakpointId'; + const debuggeeId = 'debuggeeId'; + const breakpoint: stackdriver.Breakpoint = { + id: breakpointId, + action: 'CAPTURE', + location: {path: 'foo.js', line: 99}, + } as stackdriver.Breakpoint; + const debuggee: Debuggee = {id: 'fake-debuggee'} as Debuggee; + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.debuggeeId = debuggeeId; + + let removed = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/active`).on( + 'child_removed', + data => { + assert.strictEqual(data.key, breakpointId); + removed = true; + } + ); + + let finalized = false; + db.ref(`cdbg/breakpoints/${debuggeeId}/final`).on('child_added', data => { + assert.strictEqual(data.key, breakpointId); + assert.deepStrictEqual(data.val(), { + ...breakpoint, + isFinalState: true, + finalTimeUnixMsec: {'.sv': 'timestamp'}, + }); + finalized = true; + }); + + db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( + 'child_added', + data => { + throw new Error('mock snapshot write failure'); + } + ); + + controller.updateBreakpoint(debuggee as Debuggee, breakpoint, err => { + assert(err, 'expecting an error'); + assert(removed, 'should have been removed'); + assert(!finalized, 'should not have been finalized'); + done(); + }); + }); }); }); From 007cbbd3df765b06978aa5604bcbdb4925cf725e Mon Sep 17 00:00:00 2001 From: James McTavish Date: Tue, 6 Dec 2022 09:40:14 -0500 Subject: [PATCH 3/6] fix: correctly send labels on register calls. (#1118) Debuggee labels were not being sent to the firebase rtdb when registering. This resulted in debuggees without adequate information to identify them using the snapshot debugger cli. There was no functional difference otherwise. --- src/agent/firebase-controller.ts | 2 +- test/test-firebase-controller.ts | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/agent/firebase-controller.ts b/src/agent/firebase-controller.ts index ade04399..7261b80c 100644 --- a/src/agent/firebase-controller.ts +++ b/src/agent/firebase-controller.ts @@ -169,7 +169,7 @@ export class FirebaseController implements Controller { debuggee.id = this.debuggeeId; const debuggeeRef = this.db.ref(`cdbg/debuggees/${this.debuggeeId}`); - debuggeeRef.set(debuggee, err => { + debuggeeRef.set(debuggee as {}, err => { if (err) { callback(err); } else { diff --git a/test/test-firebase-controller.ts b/test/test-firebase-controller.ts index 9b1d5ba5..4ebf7126 100644 --- a/test/test-firebase-controller.ts +++ b/test/test-firebase-controller.ts @@ -183,6 +183,40 @@ describe('Firebase Controller', () => { done(); }); }); + it('should pass labels properly', done => { + const db = new MockDatabase(); + // Debuggee Id is based on the sha1 hash of the json representation of + // the debuggee. + const debuggeeId = 'd-cbd029da'; + const debuggeeWithLabels = new Debuggee({ + project: 'fake-project', + uniquifier: 'fake-id', + description: 'unit test', + agentVersion: 'SomeName/client/SomeVersion', + labels: { + V8_version: 'v8_version', + process_title: 'node', + projectid: 'fake-project', + agent_version: '7.x', + version: 'appengine_version', + minorversion: 'minor_version', + }, + }); + + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.register(debuggeeWithLabels, (err, result) => { + assert(!err, 'not expecting an error'); + assert.ok(result); + assert.strictEqual(result!.debuggee.id, debuggeeId); + assert.strictEqual( + db.mockRef(`cdbg/debuggees/${debuggeeId}`).value, + debuggeeWithLabels + ); + done(); + }); + }); it('should error out gracefully', done => { const db = new MockDatabase(); // Debuggee Id is based on the sha1 hash of the json representation of From 42ee4a7ebcabf2a09100ed60882b276574a9ac92 Mon Sep 17 00:00:00 2001 From: "gcf-owl-bot[bot]" <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Date: Tue, 6 Dec 2022 23:44:17 +0000 Subject: [PATCH 4/6] build: have Kokoro grab service account credentials from secret that will be rotated (#1119) * build: have Kokoro grab service account credentials from secret that will be rotated Source-Link: https://togithub.com/googleapis/synthtool/commit/4a0230eb8dc497f36fd3839e6144982131f30a9d Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:f59941869d508c6825deeffce180579545fd528f359f549a80a18ec0458d7094 --- .github/.OwlBot.lock.yaml | 3 +-- .kokoro/continuous/node12/samples-test.cfg | 5 +++++ .kokoro/presubmit/node12/samples-test.cfg | 5 +++++ .kokoro/samples-test.sh | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml index 74883698..6c41b308 100644 --- a/.github/.OwlBot.lock.yaml +++ b/.github/.OwlBot.lock.yaml @@ -13,5 +13,4 @@ # limitations under the License. docker: image: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest - digest: sha256:bb493bf01d28519e82ab61c490c20122c85a7119c03a978ad0c34b4239fbad15 -# created: 2022-08-23T18:40:55.597313991Z + digest: sha256:f59941869d508c6825deeffce180579545fd528f359f549a80a18ec0458d7094 diff --git a/.kokoro/continuous/node12/samples-test.cfg b/.kokoro/continuous/node12/samples-test.cfg index e30c4f1d..ee57ba1e 100644 --- a/.kokoro/continuous/node12/samples-test.cfg +++ b/.kokoro/continuous/node12/samples-test.cfg @@ -5,3 +5,8 @@ env_vars: { key: "TRAMPOLINE_BUILD_FILE" value: "github/cloud-debug-nodejs/.kokoro/samples-test.sh" } + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/presubmit/node12/samples-test.cfg b/.kokoro/presubmit/node12/samples-test.cfg index e30c4f1d..ee57ba1e 100644 --- a/.kokoro/presubmit/node12/samples-test.cfg +++ b/.kokoro/presubmit/node12/samples-test.cfg @@ -5,3 +5,8 @@ env_vars: { key: "TRAMPOLINE_BUILD_FILE" value: "github/cloud-debug-nodejs/.kokoro/samples-test.sh" } + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/samples-test.sh b/.kokoro/samples-test.sh index fbc058a4..806c0082 100755 --- a/.kokoro/samples-test.sh +++ b/.kokoro/samples-test.sh @@ -19,7 +19,7 @@ set -eo pipefail export NPM_CONFIG_PREFIX=${HOME}/.npm-global # Setup service account credentials. -export GOOGLE_APPLICATION_CREDENTIALS=${KOKORO_GFILE_DIR}/service-account.json +export GOOGLE_APPLICATION_CREDENTIALS=${KOKORO_GFILE_DIR}/secret_manager/long-door-651-kokoro-system-test-service-account export GCLOUD_PROJECT=long-door-651 cd $(dirname $0)/.. From a421509d7c616a4ed48302498886ffa66bfefad9 Mon Sep 17 00:00:00 2001 From: James McTavish Date: Thu, 8 Dec 2022 19:15:47 -0500 Subject: [PATCH 5/6] feat: add active debuggee support (#1121) * wip: add timestamps to register call * fix: update the names of the timestamp fields * wip: working on tests; there are issues * wip: restructured and tests are passing * wip: registration complete * feat: add active debuggee support * fix: relax the test constraints for marking active debuggees * address PR feedback --- src/agent/firebase-controller.ts | 64 ++++++-- test/test-firebase-controller.ts | 261 ++++++++++++++++++++++--------- 2 files changed, 239 insertions(+), 86 deletions(-) diff --git a/src/agent/firebase-controller.ts b/src/agent/firebase-controller.ts index 7261b80c..ac503674 100644 --- a/src/agent/firebase-controller.ts +++ b/src/agent/firebase-controller.ts @@ -36,6 +36,9 @@ export class FirebaseController implements Controller { debuggeeId?: string; bpRef?: firebase.database.Reference; + markActiveInterval: ReturnType | undefined; + markActivePeriodMsec: number = 60 * 60 * 1000; // 1 hour in ms. + /** * Connects to the Firebase database. * @@ -135,7 +138,10 @@ export class FirebaseController implements Controller { } /** - * Register to the API (implementation) + * Register to the API (implementation). + * + * Writes an initial record to the database if it is not yet present. + * Otherwise only updates the last active timestamp. * * @param {!function(?Error,Object=)} callback * @private @@ -168,15 +174,29 @@ export class FirebaseController implements Controller { this.debuggeeId = `d-${debuggeeHash.substring(0, 8)}`; debuggee.id = this.debuggeeId; - const debuggeeRef = this.db.ref(`cdbg/debuggees/${this.debuggeeId}`); - debuggeeRef.set(debuggee as {}, err => { - if (err) { - callback(err); - } else { - const agentId = 'unsupported'; - callback(null, {debuggee, agentId}); - } - }); + const agentId = 'unsupported'; + // Test presence using the registration time. This moves less data. + const presenceRef = this.db.ref( + `cdbg/debuggees/${this.debuggeeId}/registrationTimeUnixMsec` + ); + presenceRef + .get() + .then(presenceSnapshot => { + if (presenceSnapshot.exists()) { + return this.markDebuggeeActive(); + } else { + const ref = this.db.ref(`cdbg/debuggees/${this.debuggeeId}`); + return ref.set({ + registrationTimeUnixMsec: {'.sv': 'timestamp'}, + lastUpdateTimeUnixMsec: {'.sv': 'timestamp'}, + ...debuggee, + }); + } + }) + .then( + () => callback(null, {debuggee, agentId}), + err => callback(err) + ); } /** @@ -300,6 +320,26 @@ export class FirebaseController implements Controller { callback(e, []); } ); + + this.startMarkingDebuggeeActive(); + } + + startMarkingDebuggeeActive() { + debuglog(`starting to mark every ${this.markActivePeriodMsec} ms`); + this.markActiveInterval = setInterval(() => { + this.markDebuggeeActive(); + }, this.markActivePeriodMsec); + } + + /** + * Marks a debuggee as active by prompting the server to update the + * lastUpdateTimeUnixMsec to server time. + */ + async markDebuggeeActive(): Promise { + const ref = this.db.ref( + `cdbg/debuggees/${this.debuggeeId}/lastUpdateTimeUnixMsec` + ); + await ref.set({'.sv': 'timestamp'}); } stop(): void { @@ -312,5 +352,9 @@ export class FirebaseController implements Controller { } catch (err) { debuglog(`failed to tear down firebase app: ${err})`); } + if (this.markActiveInterval) { + clearInterval(this.markActiveInterval); + this.markActiveInterval = undefined; + } } } diff --git a/test/test-firebase-controller.ts b/test/test-firebase-controller.ts index 4ebf7126..b52f6440 100644 --- a/test/test-firebase-controller.ts +++ b/test/test-firebase-controller.ts @@ -34,6 +34,10 @@ class MockSnapshot { val() { return this.value; } + + exists() { + return !!this.value; + } } class MockReference { @@ -45,8 +49,10 @@ class MockReference { listeners = new Map any>(); // Test options - shouldFail = false; - failMessage?: string; + shouldFailSet = false; + shouldFailGet = false; + failSetMessage?: string; + failGetMessage?: string; constructor(key: string, parentRef?: MockReference) { this.key = key.slice(); @@ -64,6 +70,14 @@ class MockReference { return Promise.resolve(); } + async get(): Promise { + if (this.shouldFailGet) { + this.shouldFailGet = false; + throw new Error(this.failGetMessage); + } + return new MockSnapshot(this.key, this.value) as {} as DataSnapshot; + } + getOrAdd(key: string): MockReference { if (!this.children.has(key)) { this.children.set(key, new MockReference(key, this)); @@ -93,12 +107,14 @@ class MockReference { } } - set(value: any, onComplete?: (a: Error | null) => any): Promise { - if (this.shouldFail) { - this.shouldFail = false; + async set(value: any, onComplete?: (a: Error | null) => any): Promise { + if (this.shouldFailSet) { + this.shouldFailSet = false; + const err = new Error(this.failSetMessage); if (onComplete) { - onComplete(new Error(this.failMessage)); + onComplete(err); } + throw err; } let creating = false; @@ -112,7 +128,6 @@ class MockReference { if (creating && this.parentRef) { this.parentRef.childAdded(this.key, value); } - return Promise.resolve(); } on( @@ -134,8 +149,13 @@ class MockReference { } failNextSet(errorMessage: string) { - this.shouldFail = true; - this.failMessage = errorMessage; + this.shouldFailSet = true; + this.failSetMessage = errorMessage; + } + + failNextGet(errorMessage: string) { + this.shouldFailGet = true; + this.failGetMessage = errorMessage; } } /* eslint-enable @typescript-eslint/no-explicit-any */ @@ -156,79 +176,142 @@ class MockDatabase { } describe('Firebase Controller', () => { - const debuggee = new Debuggee({ - project: 'fake-project', - uniquifier: 'fake-id', - description: 'unit test', - agentVersion: 'SomeName/client/SomeVersion', - }); - describe('register', () => { - it('should get a debuggeeId', done => { + const debuggee = new Debuggee({ + project: 'fake-project', + uniquifier: 'fake-id', + description: 'unit test', + agentVersion: 'SomeName/client/SomeVersion', + labels: { + V8_version: 'v8_version', + process_title: 'node', + projectid: 'fake-project', + agent_version: '7.x', + version: 'appengine_version', + minorversion: 'minor_version', + }, + }); + // Debuggee Id is based on the sha1 hash of the json representation of + // the debuggee. + const debuggeeId = 'd-cbd029da'; + + it('should error out gracefully on presence check', done => { const db = new MockDatabase(); - // Debuggee Id is based on the sha1 hash of the json representation of - // the debuggee. - const debuggeeId = 'd-008335af'; const controller = new FirebaseController( db as {} as firebase.database.Database ); - controller.register(debuggee, (err, result) => { - assert(!err, 'not expecting an error'); - assert.ok(result); - assert.strictEqual(result!.debuggee.id, debuggeeId); - assert.strictEqual( - db.mockRef(`cdbg/debuggees/${debuggeeId}`).value, - debuggee - ); - done(); + db.mockRef( + `cdbg/debuggees/${debuggeeId}/registrationTimeUnixMsec` + ).failNextGet('mocked failure'); + controller.register(debuggee, err => { + try { + assert(err, 'expecting an error'); + done(); + } catch (err) { + done(err); + } }); }); - it('should pass labels properly', done => { - const db = new MockDatabase(); - // Debuggee Id is based on the sha1 hash of the json representation of - // the debuggee. - const debuggeeId = 'd-cbd029da'; - const debuggeeWithLabels = new Debuggee({ - project: 'fake-project', - uniquifier: 'fake-id', - description: 'unit test', - agentVersion: 'SomeName/client/SomeVersion', - labels: { - V8_version: 'v8_version', - process_title: 'node', - projectid: 'fake-project', - agent_version: '7.x', - version: 'appengine_version', - minorversion: 'minor_version', - }, + describe('first time', () => { + it('should write successfully', done => { + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + const expectedDebuggee = { + ...debuggee, + registrationTimeUnixMsec: {'.sv': 'timestamp'}, + lastUpdateTimeUnixMsec: {'.sv': 'timestamp'}, + id: debuggeeId, + canaryMode: 'CANARY_MODE_UNSPECIFIED', + }; + + controller.register(debuggee, (err, result) => { + // try/catch block to avoid losing failed assertions to the error + // handling in controller.register. + try { + assert(!err, 'not expecting an error'); + assert.ok(result); + assert.strictEqual(result!.debuggee.id, debuggeeId); + assert.deepEqual( + db.mockRef(`cdbg/debuggees/${debuggeeId}`).value, + expectedDebuggee + ); + done(); + } catch (err) { + done(err); + } + }); }); - - const controller = new FirebaseController( - db as {} as firebase.database.Database - ); - controller.register(debuggeeWithLabels, (err, result) => { - assert(!err, 'not expecting an error'); - assert.ok(result); - assert.strictEqual(result!.debuggee.id, debuggeeId); - assert.strictEqual( - db.mockRef(`cdbg/debuggees/${debuggeeId}`).value, - debuggeeWithLabels + it('should error out gracefully', done => { + const db = new MockDatabase(); + db.mockRef(`cdbg/debuggees/${debuggeeId}`).failNextSet( + 'mocked failure' ); - done(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.register(debuggee, err => { + try { + assert(err, 'expecting an error'); + done(); + } catch (err) { + done(err); + } + }); }); }); - it('should error out gracefully', done => { - const db = new MockDatabase(); - // Debuggee Id is based on the sha1 hash of the json representation of - // the debuggee. - const debuggeeId = 'd-008335af'; - db.mockRef(`cdbg/debuggees/${debuggeeId}`).failNextSet('mocked failure'); - const controller = new FirebaseController( - db as {} as firebase.database.Database - ); - controller.register(debuggee, (err, result) => { - assert(err, 'expecting an error'); - done(); + describe('re-register', () => { + it('should only update the timestamp', done => { + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + // Throw an error if the debuggee is written; there should be no write. + db.mockRef(`cdbg/debuggees/${debuggeeId}`).failNextSet( + 'should not be called' + ); + // This is all that is required to indicate a prior registration. + db.mockRef(`cdbg/debuggees/${debuggeeId}/registrationTimeUnixMsec`).set( + 12345678 + ); + + controller.register(debuggee, (err, result) => { + try { + assert(!err, 'not expecting an error'); + assert.ok(result); + // In production this would be the actual timestamp. + assert.deepEqual( + db.mockRef(`cdbg/debuggees/${debuggeeId}/lastUpdateTimeUnixMsec`) + .value, + {'.sv': 'timestamp'} + ); + done(); + } catch (err) { + done(err); + } + }); + }); + it('should error out gracefully', done => { + const db = new MockDatabase(); + // This is all that is required to indicate a prior registration. + db.mockRef(`cdbg/debuggees/${debuggeeId}/registrationTimeUnixMsec`).set( + 12345678 + ); + db.mockRef( + `cdbg/debuggees/${debuggeeId}/lastUpdateTimeUnixMsec` + ).failNextSet('mocked failure'); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.register(debuggee, err => { + try { + assert(err, 'expecting an error'); + done(); + } catch (err) { + done(err); + } + }); }); }); }); @@ -279,6 +362,32 @@ describe('Firebase Controller', () => { `cdbg/breakpoints/debuggeeId/active/${breakpoints[0].id}` ).remove(); }); + + it('should start marking the debuggee as active', done => { + const db = new MockDatabase(); + const controller = new FirebaseController( + db as {} as firebase.database.Database + ); + controller.debuggeeId = 'debuggeeId'; + + controller.markActivePeriodMsec = 10; // Mark active frequently for testing purposes. + + let markedActiveCount = 0; + db.mockRef('cdbg/debuggees/debuggeeId/lastUpdateTimeUnixMsec').set = + () => { + markedActiveCount += 1; + return Promise.resolve(); + }; + + controller.subscribeToBreakpoints(debuggee, () => {}); + + // Let markActive trigger 2 times. + setTimeout(() => { + controller.stop(); + assert(markedActiveCount >= 2); + done(); + }, 50); + }); }); describe('updateBreakpoint', () => { @@ -407,20 +516,20 @@ describe('Firebase Controller', () => { db.ref(`cdbg/breakpoints/${debuggeeId}/active`).on( 'child_removed', - data => { + () => { throw new Error('mock remove failure'); } ); let finalized = false; - db.ref(`cdbg/breakpoints/${debuggeeId}/final`).on('child_added', data => { + db.ref(`cdbg/breakpoints/${debuggeeId}/final`).on('child_added', () => { finalized = true; }); let snapshotted = false; db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( 'child_added', - data => { + () => { snapshotted = true; } ); @@ -463,7 +572,7 @@ describe('Firebase Controller', () => { let snapshotted = false; db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( 'child_added', - data => { + () => { snapshotted = true; } ); @@ -512,7 +621,7 @@ describe('Firebase Controller', () => { db.ref(`cdbg/breakpoints/${debuggeeId}/snapshot`).on( 'child_added', - data => { + () => { throw new Error('mock snapshot write failure'); } ); From cb3ce240c82a5f5351089092bafc8f9836e55223 Mon Sep 17 00:00:00 2001 From: "release-please[bot]" <55107282+release-please[bot]@users.noreply.github.com> Date: Fri, 9 Dec 2022 15:54:13 +0000 Subject: [PATCH 6/6] chore(main): release 7.2.0 (#1117) :robot: I have created a release *beep* *boop* --- ## [7.2.0](https://togithub.com/googleapis/cloud-debug-nodejs/compare/v7.1.1...v7.2.0) (2022-12-09) ### Features * Add active debuggee support ([#1121](https://togithub.com/googleapis/cloud-debug-nodejs/issues/1121)) ([a421509](https://togithub.com/googleapis/cloud-debug-nodejs/commit/a421509d7c616a4ed48302498886ffa66bfefad9)) * Add error handling to firebase controller. ([#1116](https://togithub.com/googleapis/cloud-debug-nodejs/issues/1116)) ([a188e34](https://togithub.com/googleapis/cloud-debug-nodejs/commit/a188e347131168d21d01f77267863e53ae1043a8)) ### Bug Fixes * Correctly send labels on register calls. ([#1118](https://togithub.com/googleapis/cloud-debug-nodejs/issues/1118)) ([007cbbd](https://togithub.com/googleapis/cloud-debug-nodejs/commit/007cbbd3df765b06978aa5604bcbdb4925cf725e)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please). --- CHANGELOG.md | 13 +++++++++++++ package.json | 2 +- samples/package.json | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29c1a233..145b1ec4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Node.js Agent for Google Cloud Debug ChangeLog +## [7.2.0](https://github.com/googleapis/cloud-debug-nodejs/compare/v7.1.1...v7.2.0) (2022-12-09) + + +### Features + +* Add active debuggee support ([#1121](https://github.com/googleapis/cloud-debug-nodejs/issues/1121)) ([a421509](https://github.com/googleapis/cloud-debug-nodejs/commit/a421509d7c616a4ed48302498886ffa66bfefad9)) +* Add error handling to firebase controller. ([#1116](https://github.com/googleapis/cloud-debug-nodejs/issues/1116)) ([a188e34](https://github.com/googleapis/cloud-debug-nodejs/commit/a188e347131168d21d01f77267863e53ae1043a8)) + + +### Bug Fixes + +* Correctly send labels on register calls. ([#1118](https://github.com/googleapis/cloud-debug-nodejs/issues/1118)) ([007cbbd](https://github.com/googleapis/cloud-debug-nodejs/commit/007cbbd3df765b06978aa5604bcbdb4925cf725e)) + ## [7.1.1](https://github.com/googleapis/cloud-debug-nodejs/compare/v7.1.0...v7.1.1) (2022-11-18) diff --git a/package.json b/package.json index ed2bd2d5..8b41f842 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@google-cloud/debug-agent", - "version": "7.1.1", + "version": "7.2.0", "author": "Google Inc.", "description": "Stackdriver Debug Agent for Node.js", "main": "./build/src/index", diff --git a/samples/package.json b/samples/package.json index e5ea458a..2134e48a 100644 --- a/samples/package.json +++ b/samples/package.json @@ -17,7 +17,7 @@ "test": "mocha" }, "dependencies": { - "@google-cloud/debug-agent": "^7.1.1", + "@google-cloud/debug-agent": "^7.2.0", "express": "4.18.2" }, "devDependencies": {