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

feat: debug log global registrations and logger overwrites #63

Merged
merged 13 commits into from
May 31, 2021

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented May 11, 2021

Currently, there's no visibility to global registration which is a relatively important operation.
For example, there's no way to know if a logger has been overwritten or not which might mean the successive log messages/errors from other parts of the API might not be appearing without the developer's knowledge.

To relieve that, a few log statements were added:

  • There can be only one global logger. If an existing one is overwritten, both of the loggers get a warn log line about the event.
  • Any time a global is (un)registered, debug log line is created with the type and version.

It relieves the immediate pain described in open-telemetry/opentelemetry-js#3313

@rauno56 rauno56 force-pushed the feat/add-global-debugging branch from e987ff2 to a842976 Compare May 19, 2021 09:25
@rauno56 rauno56 force-pushed the feat/add-global-debugging branch from a842976 to 15f4a08 Compare May 19, 2021 09:39
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #63 (ec3675d) into main (950433f) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      open-telemetry/opentelemetry-js-api#63      +/-   ##
==========================================
+ Coverage   94.66%   94.72%   +0.06%     
==========================================
  Files          42       42              
  Lines         562      569       +7     
  Branches       94       96       +2     
==========================================
+ Hits          532      539       +7     
  Misses         30       30              
Impacted Files Coverage Δ
src/api/diag.ts 100.00% <100.00%> (ø)
src/internal/global-utils.ts 90.90% <100.00%> (+0.28%) ⬆️

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 950433f...ec3675d. Read the comment docs.

@rauno56 rauno56 marked this pull request as ready for review May 19, 2021 13:37
@dyladan
Copy link
Member

dyladan commented May 20, 2021

Really like this idea. Can be frustrating for users when they think they are loading a diag logger but it gets overwritten. Maybe it would be even worth getting the stack when it is overwritten?

// use string as fallback in case stack can't be generated
const stack = new Error().stack ?? "<failed to generate stacktrace>"

oldLogger.warn(`A new DiagLogger was registered which overwrites this logger from ${stack}`);

@rauno56
Copy link
Member Author

rauno56 commented May 21, 2021

Having a stack is a really good idea as well. I'll add that here for now to keep the focus of the PR.

Although... whenever I see variables in log messages I see a lost opportunity to structure the log entry:

  1. Thought's on using structured logging everywhere instead? An API similar to pino perhaps:
log(object, string);
log(object | string);
  1. What do you think of making it an option in the logger itself? logStackTrace: <LEVEL>/boolean. See EDIT2.

EDIT

I took the first point to discussions: #77

EDIT2

Thinking further about the global stack trace option made me think it's too much of an implementation/maintenance burden(stack traces should probably be manipulated if they are generated outside of the actual function we want to point to) for very little gain if we have structured logging.

test/internal/global.test.ts Show resolved Hide resolved
test/diag/logLevel.test.ts Outdated Show resolved Hide resolved
src/internal/global-utils.ts Show resolved Hide resolved
@rauno56
Copy link
Member Author

rauno56 commented May 31, 2021

@dyladan, @obecny. Reverted the changes to test structure. Everything should now be addressed.

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, thx for changes

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.

5 participants