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: Enshrine defra logger names #1410

Merged

Conversation

orpheuslummis
Copy link
Contributor

Relevant issue(s)

Resolves #1148

Description

Simple approach: just rename the loggers.
Suggestion: non-defra loggers could have a prefix like e.g. x.

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?

CI

@orpheuslummis orpheuslummis added the area/logging Related to the logging/logger system label Apr 27, 2023
@orpheuslummis orpheuslummis self-assigned this Apr 27, 2023
@orpheuslummis orpheuslummis force-pushed the orpheus/chore/logger-names branch from e0f47be to 388155b Compare April 27, 2023 06:00
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #1410 (78c2792) into develop (8f72069) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1410      +/-   ##
===========================================
- Coverage    72.19%   72.18%   -0.02%     
===========================================
  Files          185      185              
  Lines        18239    18239              
===========================================
- Hits         13168    13165       -3     
- Misses        4031     4033       +2     
- Partials      1040     1041       +1     
Impacted Files Coverage Δ
cli/cli.go 70.52% <ø> (ø)
config/config.go 73.30% <ø> (ø)
db/db.go 71.42% <ø> (ø)
logging/logging.go 100.00% <ø> (ø)
merkle/clock/clock.go 65.74% <ø> (ø)
merkle/crdt/merklecrdt.go 47.05% <ø> (ø)
net/api/service.go 12.50% <ø> (ø)
node/node.go 62.59% <ø> (ø)
planner/planner.go 77.37% <ø> (ø)

... and 1 file with indirect coverage changes

@orpheuslummis orpheuslummis force-pushed the orpheus/chore/logger-names branch from 388155b to 78c2792 Compare May 12, 2023 19:55
@orpheuslummis orpheuslummis marked this pull request as ready for review May 12, 2023 19:55
@orpheuslummis orpheuslummis requested a review from a team May 12, 2023 19:55
@orpheuslummis
Copy link
Contributor Author

Note that there is another approach to this which is to keep the defra.X nomenclature and just allow logger configuration from both formats defra.X and just X.

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.

I'm approving the code changes, but am unsure as to whether this is a good feature change long-term.

I can see the defra prefix as being quite handy when embedding defra. I do think explicilty hardcoding the namespaces for every logger instance is quite bad anyway though, and we should probably switch the constructor to some kind of parent-child like mechanic where the namespace is constructed without redefining the strings on every instance - is a problem for another day though, and the code changes here facilitate that somewhat.

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.

Agree with Andy that the prefix is really useful in future when defradb is embedded or used by another tool, to differentiate between defradb logs and other logs for example.

Would make a ticket for automagically handling that through the logger constructor if it won't be done in this PR.

@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented May 15, 2023

I don't plan to work on that additional idea. In my view, distinguishing between Defra logs and non-Defra logs is done at a higher level or "the process level", i.e not internal to Defra.

@orpheuslummis
Copy link
Contributor Author

That said, it is a good idea to figure out how Defra logging can play nice in the embedding case

@orpheuslummis orpheuslummis merged commit db4b853 into sourcenetwork:develop May 15, 2023
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1148

## Description

Simple approach: just rename the loggers.

## How has this been tested?

CI & manual
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logging Related to the logging/logger system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger names are prepended with defra. make them less usable
3 participants