Skip to content
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

Centralized logger implementation #53

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

namark
Copy link
Contributor

@namark namark commented Oct 30, 2021

used minimally just to solve #45

src/domain/audio-client/AudioClient.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
tests/domain/shared/Log.unit.test.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
src/domain/shared/Log.ts Outdated Show resolved Hide resolved
@namark
Copy link
Contributor Author

namark commented Nov 22, 2021

Summary of recent updates.

SDK user facing changes:
New LoggerConfiguration interface contains all the logger configuration for the user - output context and filters. SDK wide configuration is exposed in DefaultLoggerConfiguration object.
New LoggerContext implementations allow to easily combine other context to log to multiple destinations, apply a filter per destination and buffer messages until an error is logged.
Main SDK logger is now prefixed with "vircadia-sdk" tag, and by default filtered to only log warning and error messages (will only have an effect once existing console logs are all replaced)

SDK developer facing changes:
Message type parameter has been replaced with tag state changing function, which also supports multiple tags.
Various logging functions now accept multiple objects to log constrained to specific list of types.
The one time logging function is replaced with a state changing function and a getter for ease of use.

Regarding string and regexp filtering, I realized that the behavior would be context dependent. For example currently the console context just forwards parameters to the dev console, without ever combining them to a single string to apply a string filter to. Also even just conceptually for example the log level is not represented as text there, while it would be if logging to a file, which would change the behavior of such filters between these two contexts. So if we ever want to use this code for a user facing GUI log with string/regexp filter, we'll need to implement an appropriate context, which will itself provide the filtering functionality in whichever way it would make the most sense there.

@ctrlaltdavid
Copy link
Collaborator

  • Please rename \tests\shared\Log.unit.test.ts to Log.unit.test.js (i.e., JavaScript - it doesn't appear to use any TypeScript features).

  • Please move \domain\shared\Log.ts into \shared\Log.ts. (The \domain folder is primarily for C++-equivalent domain server code.) Similarly move the corresponding Jest test file.

  • Unit tests are missing for the global Log object.

  • The JSDoc for types and interfaces is not coming through in the dev/sdk docs. To fix this, the JSDoc for types and interfaces need to be included inside a class (e.g., the main class where they're used).
    For example, the documentation for the following types doesn't appear:

    • LoggerConfiguration
    • LoggableObjectRecord

    For an example of including a type's JSDoc inside a class, see: \domain\networking\packets\AvatarData.ts.

  • It would be helpful to include in the PR description a list of the ways it is envisaged that the global Log object be used in practice.

@namark
Copy link
Contributor Author

namark commented Nov 26, 2021

The Log object is meant to be used for logging purposed in the SDK, to replace all the current and future console logs. Similar to console it provides a logging function per level (debug, message, info, warning, error), and additionally a function that accepts a LogLevel enum which defines the logging levels and their numeric priority (can be expanded in future). The type of objects that can be logged is constrained to simplify the implementation of various backends (through LoggerContext interface), while minimizing any loss of information. Basic use example:

Log.warning("something's not right about these values", {value1: 13, value2: "friday"});

For structured filtering purposes in addition to log level the logger also implements tagging.

Log.tag("my-stuff", "important").message("my message");

The tag function accepts a list of strings returns a new logger instance that will include those tags with every message logged. If the given set of tags is used often, the new logger instance can be reused to avoiding repeating the tags.

const networkLog = Log.tag("networking");
...
networkingLog.error("Connection failed", ...);

Specifically to resolve #45 the logger implements prevention of duplicate messages, also using method chaining:

Log.one.warning("not implemented");

For completeness there is a once function that accepts a boolean enabled flag, but no specific use case for that.

The Log object is instantiated with DefaultLoggerConfiguration that specifies the output context and filters, and is exposed to SDK user for customization. Currently the defaults are, ConsoleLoggerContext which logs to dev console, and a simple filter to allow only warnings and errors, but in future this might be different between production and development or other build settings. The user can specify filters based on log levels, tags, or the full message, and specify the output by either using the LoggerContext implementations provided by the SDK, or implementing their own, which should be relatively straight forward, it's a single function with a well defined parameters to handle.

Copy link
Collaborator

@ctrlaltdavid ctrlaltdavid left a comment

Choose a reason for hiding this comment

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

Suggested changes fix npm run lint errors.

Additionally, npm run test .unit. fails with error:

TypeError: [console.debug,console.log,console.info,console.warn,console.error].at is not a function

tests/shared/Log.unit.test.js Show resolved Hide resolved
tests/shared/Log.unit.test.js Outdated Show resolved Hide resolved
@ctrlaltdavid ctrlaltdavid added the enhancement New feature or request label Nov 27, 2021
example/interface.js Show resolved Hide resolved
example/interface.js Show resolved Hide resolved
@Misterblue
Copy link
Contributor

I have tested with this PR and have had no problems. I approve this PR

@Misterblue
Copy link
Contributor

Ping! This does not merge into the master branch any more. Changes are needed.

@digisomni
Copy link
Member

I believe David is adjusting it.

@ctrlaltdavid
Copy link
Collaborator

ctrlaltdavid commented Dec 3, 2021

Yes, I'm looking at this. It will need some further work (to be determined) before being merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce console log spam
4 participants