-
Notifications
You must be signed in to change notification settings - Fork 51
feat: Add ability to store breakpoint data in firebase realtime database #1067
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left mostly nits, this code reads fairly clean to me.
@@ -412,4 +427,6 @@ export const defaultConfig: ResolvedDebugAgentConfig = { | |||
forceNewAgent_: false, | |||
testMode_: false, | |||
resetV8DebuggerThreshold: 30, | |||
|
|||
useFirebase: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were ever to make firebase
the default, would we remove this toggle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we would, and in the process would also retire a number of config values that would no longer apply.
src/agent/debuglet.ts
Outdated
'node version': process.versions.node, | ||
'V8 version': process.versions.v8, | ||
'agent.name': packageInfo.name, | ||
'agent.version': packageInfo.version, | ||
'agent name': packageInfo.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given all the restructuring, I might call it a breaking change, even if there's no one specific breakage we're calling attention to.
@@ -50,6 +50,7 @@ | |||
"console-log-level": "^1.4.0", | |||
"extend": "^3.0.2", | |||
"findit2": "^2.2.3", | |||
"firebase-admin": "^9.11.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -85,7 +86,7 @@ describe('Controller', function () { | |||
|
|||
it('should pass success on timeout', done => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some system tests for the firebase real time database controller, non blocker for this initial PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This is a work in progress:
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:
Fixes #<issue_number_goes_here> 🦕