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

chore: Add configuration of external loggers #942

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

fredcarle
Copy link
Collaborator

Relevant issue(s)

Resolves #729
Resolves #710

Description

This PR adds support for external loggers. Specifically go-log (imported by a dependancy) and go-log V2 (imported by datastore/badger/v3). It adds a mechanism to update the external logger with the provided configuration.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

unit test and visual

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added area/logging Related to the logging/logger system action/no-benchmark Skips the action that runs the benchmark. labels Nov 14, 2022
@fredcarle fredcarle added this to the DefraDB v0.4 milestone Nov 14, 2022
@fredcarle fredcarle requested a review from a team November 14, 2022 02:58
@fredcarle fredcarle self-assigned this Nov 14, 2022
logging/logger.go Outdated Show resolved Hide resolved
Comment on lines +265 to +290
// goLogger is a wrapper for a go-log logger
// Used by github.com/ipfs/go-ipfs-provider
type goLogger struct {
*logger
*golog.ZapEventLogger
}

func GetGoLogger(name string) *goLogger {
l := mustNewLogger(name)
gl := golog.Logger(name)
return &goLogger{
logger: l,
ZapEventLogger: gl,
}
}

func (l *goLogger) ApplyConfig(config Config) {
l.logger.ApplyConfig(config)
l.ZapEventLogger.SugaredLogger = *l.logger.logger.Sugar()
}
Copy link
Member

Choose a reason for hiding this comment

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

question: The wrapper code seems fairly similar for both the loggers, what is the difference between the loggers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are pretty much the same. It's just that both are imported so both need to be handled as they have their own package specific map of registered loggers.

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #942 (98d08e3) into develop (3e69fe6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #942      +/-   ##
===========================================
+ Coverage    60.30%   60.34%   +0.03%     
===========================================
  Files          164      164              
  Lines        17712    17732      +20     
===========================================
+ Hits         10682    10701      +19     
- Misses        6082     6083       +1     
  Partials       948      948              
Impacted Files Coverage Δ
logging/registry.go 100.00% <ø> (ø)
logging/logger.go 78.68% <100.00%> (+2.40%) ⬆️
connor/lt.go 72.72% <0.00%> (-4.55%) ⬇️

Copy link
Contributor

@orpheuslummis orpheuslummis left a comment

Choose a reason for hiding this comment

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

LGTM, but I suggest (non-blocking) to add a comment addressing adding external loggers that lead to ambiguous selectors - when the external logger's funcs/fields conflicts with the internal logger's interface (if I'm understanding this correctly).

@fredcarle
Copy link
Collaborator Author

LGTM, but I suggest (non-blocking) to add a comment addressing adding external loggers that lead to ambiguous selectors - when the external logger's funcs/fields conflicts with the internal logger's interface (if I'm understanding this correctly).

Thanks.

As for your suggestion, I'm not sure it's relevant. We don't care about the funcs/fields of the external logger. All we care about is ensuring it takes the configs that we send it. If you think I'm misunderstanding please let me know and maybe add a bit more details so I can get a better idea.

@orpheuslummis
Copy link
Contributor

LGTM, but I suggest (non-blocking) to add a comment addressing adding external loggers that lead to ambiguous selectors - when the external logger's funcs/fields conflicts with the internal logger's interface (if I'm understanding this correctly).

Thanks.

As for your suggestion, I'm not sure it's relevant. We don't care about the funcs/fields of the external logger. All we care about is ensuring it takes the configs that we send it. If you think I'm misunderstanding please let me know and maybe add a bit more details so I can get a better idea.

All I'm suggesting is to add one comment guiding the future developer adding an external logger there - if you see that as valuable.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM

@fredcarle fredcarle force-pushed the fredcarle/chore/I729-external-logger-config branch from 4b5f34f to 98d08e3 Compare November 16, 2022 18:41
@fredcarle
Copy link
Collaborator Author

All I'm suggesting is to add one comment guiding the future developer adding an external logger there - if you see that as valuable.

comment added.

@fredcarle fredcarle merged commit 4b14839 into develop Nov 16, 2022
@fredcarle fredcarle deleted the fredcarle/chore/I729-external-logger-config branch November 16, 2022 19:00
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Relevant issue(s)
Resolves sourcenetwork#729
Resolves sourcenetwork#710

Description
This PR adds support for external loggers. Specifically go-log (imported by a dependancy) and go-log V2 (imported by datastore/badger/v3). It adds a mechanism to update the external logger with the provided configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/logging Related to the logging/logger system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration of compatible loggers Error logged from go-ipfs-provider dependency when no peer is provided
4 participants