Skip to content
This repository was archived by the owner on Nov 17, 2025. It is now read-only.

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?

@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.

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

4 participants