Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

chore: refactor diag logger #9

Merged
merged 7 commits into from Feb 25, 2021
Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Feb 24, 2021

/cc @MSNev

Per request of @Flarna this is the diag refactoring split out of #6. There may be some relevant discussion in the previous PR.

This PR is a prerequisite for #10

  • The filtered logger is now the only global that is saved
    • log level and the logger are now enclosed in the filtered logger
  • setLogLevel is now removed in favor of a second optional argument to setLogger
  • setLogger()/setLogger(undefined)/setLogger(null) are now disallowed and replaced with disable
    • Calling this way anyway results in a noop logger
  • Constructing a log level filtered logger results in a logger which ignores the global log level and only uses the one it is specifically constructed with
  • Methods to create log level loggers and noop loggers are now not exported to the user and treated as internal concerns. The only exports are diag, DiagLogger, DiagLogLevel, and LogFunction.

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #9 (df18a12) into main (ed9ba80) will decrease coverage by 0.49%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
- Coverage   94.27%   93.77%   -0.50%     
==========================================
  Files          35       37       +2     
  Lines         489      466      -23     
  Branches       78       75       -3     
==========================================
- Hits          461      437      -24     
- Misses         28       29       +1     
Impacted Files Coverage Δ
src/diag/consoleLogger.ts 100.00% <ø> (ø)
src/api/diag.ts 97.43% <95.65%> (-2.57%) ⬇️
src/diag/index.ts 100.00% <100.00%> (ø)
src/diag/internal/logLevelLogger.ts 100.00% <100.00%> (ø)
src/diag/internal/noopLogger.ts 100.00% <100.00%> (ø)
src/diag/types.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed9ba80...ecc34e8. Read the comment docs.

@dyladan
Copy link
Member Author

dyladan commented Feb 24, 2021

@MSNev I can't add you as a reviewer because it seems you aren't a member of the OpenTelemetry community?

src/diag/logger.ts Outdated Show resolved Hide resolved
@dyladan
Copy link
Member Author

dyladan commented Feb 24, 2021

Ok, I know I said id mention that "m" word less, buuuutttt...

Does this result in smaller output? Looks like it might be close -- enough?

As this definitely looks easier to read. And while I can think of a few additional tricks, I'm not sure its worth it, basically

  • keep the diagLoggerFunctions but don't export from main project (effectively keeping internal)
  • still use for loop for diag.ts, here (which some funcky toUpper() for the level lookup) and in the noopLoger.ts.

The looping might trim a tiny bit off, but you also add the array itself, the log level map, and the import statements. I think the readability alone is a good argument to keep it this way, but the minification win just isn't big enough IMO.

src/api/diag.ts Outdated Show resolved Hide resolved
src/api/diag.ts Show resolved Hide resolved
src/diag/internal/logLevelLogger.ts Outdated Show resolved Hide resolved
Copy link
Member

@obecny obecny 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
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

looks good -- just added 1 suggestion

@MSNev
Copy link
Contributor

MSNev commented Feb 25, 2021

@MSNev I can't add you as a reviewer because it seems you aren't a member of the OpenTelemetry community?

open-telemetry/community#666

@dyladan dyladan merged commit fb98977 into open-telemetry:main Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants