feat: Add ability to store breakpoint data in firebase realtime database by mctavish · Pull Request #1067 · googleapis/cloud-debug-nodejs

mctavish · 2022-04-22T21:39:13Z

This is a work in progress:

Firebase-related code may need updates to hit desired code quality.
Firebase-related code does not yet have unit tests
Some OnePlatform-related tests have been temporarily disabled.
Documentation has not been updated

The intention is to get reviews going on this code as development continues on this branch. Once the above issues (and any others that come up in review) are addressed, the branch can be pulled into main.

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:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

Work still needed on the controller interface to make it compatible. Registration works, but ListActiveBreakpoints is not working. UpdateBreakpoint is untested. Error handling and tests have not been handled yet.

- We now use a top level 'cdbg' node - Add timestamp with breakpoint is finalized - When a snapshot triggers, a separate node is used for the complete snapshot data under final.

Also disable the tests that aren't working yet, and remove a bunch of console.log messages.

This function isn't used by the debuglet, but is used by the oneplatform controller tests.

codecov · 2022-04-22T22:23:47Z

Codecov Report

Merging #1067 (612669a) into firebase-backend (a4add51) will decrease coverage by 3.39%.
The diff coverage is 55.06%.

@@                 Coverage Diff                  @@
##           firebase-backend    #1067      +/-   ##
====================================================
- Coverage             66.86%   63.47%   -3.40%     
====================================================
  Files                    20       21       +1     
  Lines                  1648     1733      +85     
  Branches                335      346      +11     
====================================================
- Hits                   1102     1100       -2     
- Misses                  469      553      +84     
- Partials                 77       80       +3

Impacted Files	Coverage Δ
src/agent/config.ts	`100.00% <ø> (ø)`
src/agent/firebase-controller.ts	`9.37% <9.37%> (ø)`
src/index.ts	`83.33% <57.14%> (-11.41%)`	⬇️
src/agent/debuglet.ts	`76.51% <72.00%> (-3.99%)`	⬇️
src/agent/oneplatform-controller.ts	`74.52% <74.52%> (ø)`

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 a4add51...612669a. Read the comment docs.

bcoe

Left mostly nits, this code reads fairly clean to me.

bcoe · 2022-05-04T15:23:37Z

src/agent/config.ts

  testMode_: false,
  resetV8DebuggerThreshold: 30,
+
+  useFirebase: false,


If we were ever to make firebase the default, would we remove this toggle?

Yes we would, and in the process would also retire a number of config values that would no longer apply.

src/agent/debuglet.ts

bcoe · 2022-05-04T15:32:59Z

src/agent/debuglet.ts

      'V8 version': process.versions.v8,
-      'agent.name': packageInfo.name,
-      'agent.version': packageInfo.version,
+      'agent name': packageInfo.name,


We should probably call this a breaking change, I'm assuming the issue is that firebase can't handle a . in keynames?

Perhaps we could use a _ rather than a , that reads a bit better to me and is easier to search.

Correct; and I like the underscore idea.

This shouldn't be a breaking change (at least for users of the agent) but given the addition of the configuration and new dependencies this feature could be considered to be significant enough to warrant a major version bump. Thoughts?

Given all the restructuring, I might call it a breaking change, even if there's no one specific breakage we're calling attention to.

src/agent/debuglet.ts

src/agent/firebasecontroller.ts

src/agent/oneplatformcontroller.ts

test/test-debuglet.ts

bcoe · 2022-05-04T15:40:53Z

package.json

    "console-log-level": "^1.4.0",
    "extend": "^3.0.2",
    "findit2": "^2.2.3",
+    "firebase-admin": "^9.11.1",


This dependency is huge, and I've heard can hurt cold start times. Not a blocker, but might be worth looking to see if the firebase team has a library specifically for the real time database.

We're pulling in the app, credentials, and database portions of the dependency, and there don't appear to be more focused distributions of these libraries. The firebase-admin package is much smaller than the regular firebase one though, so hopefully the impact will be less.

There aren't any ways to hint to node that we only care about those three parts of the package, are there?

bcoe · 2022-05-04T15:41:33Z

system-test/test-controller.ts

@@ -85,7 +86,7 @@ describe('Controller', function () {

  it('should pass success on timeout', done => {


We should add some system tests for the firebase real time database controller, non blocker for this initial PR.

Agreed! The challenge that I have at the moment is finding a suitable mock for the database. nock doesn't do the trick since the client isn't using the rest api. Any suggestions?

mctavish and others added 13 commits September 10, 2021 16:06

Checkpoint on Firebase controller.

b0c7c19

Work still needed on the controller interface to make it compatible. Registration works, but ListActiveBreakpoints is not working. UpdateBreakpoint is untested. Error handling and tests have not been handled yet.

Add missing controller implementations

5e55af2

Create breakpoint subscription interface. Needs significant work still.

3b427bf

Move api key and db url to the client's control

ba0654e

Some changes to work with the cli implementation

802bc11

- We now use a top level 'cdbg' node - Add timestamp with breakpoint is finalized - When a snapshot triggers, a separate node is used for the complete snapshot data under final.

Update oneplatform re-registration logic.

bd56e10

Merge branch 'googleapis:main' into firebase

1368371

Fix tests that relied on debuglet.stop().

d56f9a8

Also disable the tests that aren't working yet, and remove a bunch of console.log messages.

Remove listBreakpoints from controller interface.

f4c2176

This function isn't used by the debuglet, but is used by the oneplatform controller tests.

Remove unused code

69ead57

Remove stale broken test indicators.

3226771

Remove stale comments

e1c9f57

Fix some lint and formatting issues.

ac40efe

mctavish requested a review from a team April 22, 2022 21:39

mctavish assigned bcoe Apr 22, 2022

mctavish requested a review from a team April 22, 2022 21:39

product-auto-label bot added size: l Pull request size is large. api: clouddebugger Issues related to the googleapis/cloud-debug-nodejs API. labels Apr 22, 2022

Fix type of breakpoint_map

eeef44e

mctavish added 3 commits April 25, 2022 09:47

Clear some linter errors.

0489e62

Update copyright year

15b1f88

Improve firebase client initialization.

35458f3

bcoe suggested changes May 4, 2022

View reviewed changes

mctavish added 4 commits May 4, 2022 14:18

Remove unnecessary that = this.

8dcc46d

Rename controller implementation files.

823df05

Fix some comments

ab2f1eb

Use underscores in label keys.

e0d2cea

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels May 4, 2022

mctavish added 2 commits May 4, 2022 15:41

Fix linter error.

3f4c37d

Replace console.log with util.debuglog.

612669a

mctavish linked an issue May 5, 2022 that may be closed by this pull request

Add ability to store breakpoint data in firebase realtime database #1071

Closed

This was referenced May 5, 2022

Firebase-backend branch: Re-enable unit tests related to ready state detection #1072

Closed

Add ability to store breakpoint data in firebase realtime database #1071

Closed

mctavish requested a review from bcoe May 5, 2022 14:59

bcoe approved these changes May 5, 2022

View reviewed changes

mctavish merged commit 22e255a into googleapis:firebase-backend May 5, 2022

		@@ -85,7 +86,7 @@ describe('Controller', function () {

		it('should pass success on timeout', done => {

Conversation

mctavish commented Apr 22, 2022

Uh oh!

codecov bot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bcoe left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Apr 22, 2022 •

edited

Loading