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.

Sign up for GitHub

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

Jump to bottom

Consolidate logging and introduce auto-prefix #7731

Merged
merged 6 commits into from
May 17, 2023
Merged

Consolidate logging and introduce auto-prefix #7731

merged 6 commits into from
May 17, 2023

Conversation

Iku-turso
Copy link
Contributor

Auto-prefix means that when in Feature with id "some-feature", usages of eg. logErrorInjectionToken will have [SOME-FEATURE]: as prefix.

eg.

// Somewhere within "some-feature"...
const logError = di.inject(logErrorInjectionToken);
logError("some-error"); // -> "[SOME-FEATURE]: some-error"

@Iku-turso Iku-turso requested a review from a team as a code owner May 16, 2023 13:26
@Iku-turso Iku-turso requested review from ixrock and gabriel-mirantis and removed request for a team May 16, 2023 13:26
@Iku-turso Iku-turso added enhancement New feature or request chore labels May 16, 2023
@Iku-turso Iku-turso modified the milestones: 6.6.0, 6.5.0 May 16, 2023
@Iku-turso Iku-turso force-pushed the consolidate-logging-and-introduce-autoprefix branch from 257aa46 to 2eb7a0e Compare May 16, 2023 13:33
@Iku-turso Iku-turso removed the enhancement New feature or request label May 16, 2023
Iku-turso
Iku-turso commented May 16, 2023
View reviewed changes
packages/logger/src/logger.injectable.ts
return (message, ...data) => {
winstonLogger[scenario](
di.sourceNamespace
? `[${screamingKebabCase(di.sourceNamespace)}]: ${message}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice: di.sourceNamespace is a new thing in injectable, making this auto-prefixing possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use the already existing prefixedLogger?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because after this you don't have to worry about prefixing at all anymore. It's all done automatically within loggerFeature. Usage doesn't have to know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of inconsistency, boilerplate and unnecessary complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More interestingly, I'm thinking how di.sourceNamespace could be leveraged elsewhere. With it, we can have stuff that is guaranteed to have a Feature-specific instance.

  1. We can simplify injections eg. for less required instantiationParameters.
  2. We can have error handling that can identify problematic Features.
  3. We can have profiling that can identify slow Features.
  4. We can have Features able to only access its own directory in file-system.
  5. We can solve many naming collision issues, as namespace is always guaranteed unique.
  6. ...best ideas win a prize at next team event ;)

Copy link
Contributor

@gabriel-mirantis gabriel-mirantis May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

profile the loading time of the app so we can get closer to sub 2second loading
and send that someplace that can be seen from metabase.
I'd like to be able to see the mean load time across the userbase by release

Nokel81
Nokel81 reviewed May 16, 2023
View reviewed changes
packages/logger/src/logger.injectable.ts
Comment on lines +27 to +55
export type LogFunction = (message: string, ...data: any[]) => void;

export const logDebugInjectionToken = getInjectionToken<LogFunction>({
id: "log-debug-injection-token",
});

export const logInfoInjectionToken = getInjectionToken<LogFunction>({
id: "log-info-injection-token",
});

export const logWarningInjectionToken = getInjectionToken<LogFunction>({
id: "log-warning-injection-token",
});

export const logErrorInjectionToken = getInjectionToken<LogFunction>({
id: "log-error-injection-token",
});

export const logSillyInjectionToken = getInjectionToken<LogFunction>({
id: "log-silly-injection-token",
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also why are these categorically better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending from something that you don't use completely is considered as bad practice. (Interface segregation principle) E.g. if you depend from logger which is object containing multiple methods, you'll likely use only one of them. This means that e.g. in the unit tests you have to override stuff that you are not interested.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Iku-turso and others added 6 commits May 17, 2023 11:11
@Iku-turso @jansav
feat: Expose different ways to log as tokens, and add unit tests
3651a82 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso @jansav
chore: Bump ogre-tools to permit Feature-specific logging prefixes
b8a81c6 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso @jansav
feat: Implement automatic Feature-specific prefixing for logging
4c7177b 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso @jansav
chore: Consolidate to using a shared NPM-script
c7216a6 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso @jansav
chore: Deprecate obsolete ways to log
b126592 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso @jansav
chore: Update package-lock.json
1dec61f 
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso Iku-turso force-pushed the consolidate-logging-and-introduce-autoprefix branch from 2eb7a0e to 1dec61f Compare May 17, 2023 08:30
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Iku-turso Iku-turso merged commit fc641b3 into master May 17, 2023
@Iku-turso Iku-turso deleted the consolidate-logging-and-introduce-autoprefix branch May 17, 2023 08:55
@k8slens-bot k8slens-bot mentioned this pull request May 18, 2023
Closed
@Nokel81 Nokel81 mentioned this pull request May 19, 2023
Closed
This was referenced May 23, 2023
Closed
Closed
@Nokel81 Nokel81 mentioned this pull request May 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jansav jansav jansav left review comments

@Nokel81 Nokel81 Nokel81 left review comments

@gabriel-mirantis gabriel-mirantis gabriel-mirantis approved these changes

@ixrock ixrock Awaiting requested review from ixrock ixrock is a code owner automatically assigned from lensapp/lens-ide

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Iku-turso @jansav @Nokel81 @gabriel-mirantis