Warn when stream is closing due to error, not debug. #3064

wu-hui · 2020-05-14T17:56:06Z

b/155897356

schmidt-sebastian

Let's chat about this quickly in API discussion. We currently don't have a way for users to set the log level to "warn": https://firebase.google.com/docs/reference/js/firebase.firestore#setloglevel

google-oss-bot · 2020-05-23T23:46:37Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`67adfb2`)	Head (`0b1a8f2`)	Diff
browser	251 kB	250 kB	-1.16 kB (-0.5%)
esm2017	195 kB	194 kB	-911 B (-0.5%)
main	494 kB	492 kB	-2.01 kB (-0.4%)
module	249 kB	248 kB	-1.21 kB (-0.5%)

@firebase/firestore/lite

Type Base (67adfb2) Head (0b1a8f2) Diff

main ? 6.51 kB ? (?)

`@firebase/firestore/memory`

Type	Base (`67adfb2`)	Head (`0b1a8f2`)	Diff
browser	192 kB	191 kB	-810 B (-0.4%)
esm2017	149 kB	149 kB	-676 B (-0.5%)
main	370 kB	368 kB	-1.26 kB (-0.3%)
module	190 kB	189 kB	-853 B (-0.4%)

`@firebase/util`

Type	Base (`67adfb2`)	Head (`0b1a8f2`)	Diff
browser	19.5 kB	19.6 kB	+143 B (+0.7%)
esm2017	17.4 kB	17.5 kB	+126 B (+0.7%)
main	19.5 kB	19.6 kB	+143 B (+0.7%)
module	18.6 kB	18.7 kB	+126 B (+0.7%)

`firebase`

Type	Base (`67adfb2`)	Head (`0b1a8f2`)	Diff
firebase-firestore.js	290 kB	289 kB	-1.21 kB (-0.4%)
firebase-firestore.memory.js	232 kB	231 kB	-859 B (-0.4%)
firebase.js	823 kB	822 kB	-1.21 kB (-0.1%)

Test Logs

Base (67adfb2c): https://github.com/firebase/firebase-js-sdk/actions/runs/117281137
Head (0b1a8f21): https://github.com/firebase/firebase-js-sdk/actions/runs/121851551

schmidt-sebastian

Thanks for updating this. Please remember to get sign off by the API council.

schmidt-sebastian · 2020-05-26T16:21:50Z

packages/firestore/src/api/database.ts

-        // The default log level is error
-        return 'error';
-    }
+  static get logLevel(): LogLevel {


Can you change this back to firestore.LogLevel, remove the use of the LogLevel type (we should only use LogLevelString) and change the typings for firestore.LogLevel to match the log types in Util?

schmidt-sebastian · 2020-05-26T16:23:11Z

packages/firestore/src/util/log.ts

 import { SDK_VERSION } from '../core/version';
 import { PlatformSupport } from '../platform/platform';

-export { LogLevel };
+export { LogLevel, LogLevelString };


You probably don't need to export these anymore. Instead, you can rely on the fact that our typings now match the types in @firebase/logger.

static get logLevel(): firestore.LogLevel from database.ts still needs it to return the string values.

schmidt-sebastian · 2020-05-26T16:25:43Z

packages/firestore/src/api/database.ts

  }

-  static setLogLevel(level: firestore.LogLevel): void {
+  static setLogLevel(level: LogLevelString): void {
    validateExactNumberOfArgs('Firestore.setLogLevel', arguments, 1);
    validateArgType('Firestore.setLogLevel', 'non-empty string', 1, level);


Instead of this, you may want to call https://osscs.corp.google.com/firebase-sdk/firebase-js-sdk/+/master:packages/firestore/src/util/input_validation.ts;l=303?q=validate%20enum&ss=firebase-sdk%2Ffirebase-js-sdk

If you don't validate the enum constant here, setLogLevel sets the log level to undefined.

# Conflicts: # packages/logger/src/logger.ts

schmidt-sebastian · 2020-05-29T04:25:44Z

packages/firestore-types/index.d.ts

@@ -37,7 +37,7 @@ export interface PersistenceSettings {
  experimentalTabSynchronization?: boolean;
 }

-export type LogLevel = 'debug' | 'error' | 'silent';
+export type LogLevel = 'debug' | 'error' | 'silent' | 'warn';


Do we not want to expose all levels? You are missing verbose and info.

schmidt-sebastian · 2020-05-29T04:28:18Z

packages/firestore/src/util/log.ts

@@ -28,8 +28,8 @@ export function getLogLevel(): LogLevel {
  return logClient.logLevel;
 }

-export function setLogLevel(newLevel: LogLevel): void {
-  logClient.logLevel = newLevel;
+export function setLogLevel(newLevel: LogLevelString | LogLevel): void {


You can probably drop LogLevel as an input type.

async_queue.test.ts calls getLogLevel() before changing it, then set the saved log level back when test is done. https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/test/unit/util/async_queue.test.ts#L84

It's only testing code, but i think it does no harm to keep it anyways?

SGTM.

This could also be fixed by returning the LogLevelString from getLogLevel() and calling setLogLevel('silent') instead of setLogLevel(LogLevel.SILENT). We can leave as is though.

schmidt-sebastian · 2020-05-29T16:40:44Z

packages/firestore/src/api/database.ts

-    }
+    validateStringEnum(
+      'setLogLevel',
+      ['debug', 'error', 'silent', 'warn'],


Please add 'verbose' and 'info' here as well.

schmidt-sebastian · 2020-05-29T16:45:34Z

packages/firestore/src/util/log.ts

@@ -28,8 +28,8 @@ export function getLogLevel(): LogLevel {
  return logClient.logLevel;
 }

-export function setLogLevel(newLevel: LogLevel): void {
-  logClient.logLevel = newLevel;
+export function setLogLevel(newLevel: LogLevelString | LogLevel): void {


SGTM.

This could also be fixed by returning the LogLevelString from getLogLevel() and calling setLogLevel('silent') instead of setLogLevel(LogLevel.SILENT). We can leave as is though.

Feiyang1 · 2020-06-02T21:32:16Z

packages/firestore-types/index.d.ts

+export type LogLevel =
+  | 'debug'
+  | 'error'
+  | 'silent'
+  | 'warn'
+  | 'info'
+  | 'verbose';


Are you planning to make it public? If so, you have to update packages/firebase/index.d.ts

@wu-hui @schmidt-sebastian

Pinged on chat

wu-hui added the api: firestore label May 14, 2020

wu-hui requested a review from schmidt-sebastian May 14, 2020 17:56

wu-hui assigned schmidt-sebastian May 14, 2020

schmidt-sebastian reviewed May 14, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 14, 2020

wu-hui requested review from Feiyang1, hiranya911 and hsubox76 as code owners May 23, 2020 23:38

wu-hui added 2 commits May 23, 2020 19:54

Warn when stream is closing due to error, not debug.

74f3e84

Add setLogLevel

db25c5f

wu-hui force-pushed the wuandy/WarnOnStreamError branch from 6722078 to db25c5f Compare May 24, 2020 00:09

wu-hui assigned hsubox76 and schmidt-sebastian and unassigned wu-hui May 25, 2020

schmidt-sebastian reviewed May 26, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 26, 2020

wu-hui added 2 commits May 28, 2020 10:21

Merge branch 'master' into wuandy/WarnOnStreamError

8825bd5

# Conflicts: # packages/logger/src/logger.ts

Improvements.

7547bb2

wu-hui assigned schmidt-sebastian and unassigned hsubox76 and wu-hui May 29, 2020

schmidt-sebastian approved these changes May 29, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 29, 2020

Add all levels.

d102dd1

wu-hui assigned schmidt-sebastian and unassigned wu-hui May 29, 2020

schmidt-sebastian suggested changes May 29, 2020

View reviewed changes

Add missing string values for log level

e71188c

wu-hui assigned schmidt-sebastian and unassigned schmidt-sebastian Jun 1, 2020

schmidt-sebastian approved these changes Jun 1, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 1, 2020

wu-hui merged commit 31f7afc into master Jun 1, 2020

Feiyang1 reviewed Jun 2, 2020

View reviewed changes

firebase locked and limited conversation to collaborators Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when stream is closing due to error, not debug. #3064

Warn when stream is closing due to error, not debug. #3064

wu-hui commented May 14, 2020 •

edited

Loading

schmidt-sebastian left a comment

google-oss-bot commented May 23, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`@firebase/util`

`firebase`

schmidt-sebastian left a comment

schmidt-sebastian May 26, 2020

wu-hui May 28, 2020

schmidt-sebastian May 26, 2020

wu-hui May 28, 2020

schmidt-sebastian May 26, 2020

wu-hui May 28, 2020

schmidt-sebastian May 29, 2020

wu-hui May 29, 2020

schmidt-sebastian May 29, 2020

wu-hui May 29, 2020

schmidt-sebastian May 29, 2020

schmidt-sebastian May 29, 2020

wu-hui Jun 1, 2020

schmidt-sebastian May 29, 2020

Feiyang1 Jun 2, 2020

Feiyang1 Jun 4, 2020

schmidt-sebastian Jun 4, 2020

Warn when stream is closing due to error, not debug. #3064

Warn when stream is closing due to error, not debug. #3064

Conversation

wu-hui commented May 14, 2020 • edited Loading

schmidt-sebastian left a comment

Choose a reason for hiding this comment

google-oss-bot commented May 23, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/lite

@firebase/firestore/memory

@firebase/util

firebase

Test Logs

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hui commented May 14, 2020 •

edited

Loading

google-oss-bot commented May 23, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`@firebase/util`

`firebase`