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

Logger: Add support for toggling sub logger name output for log events #407

Merged
merged 7 commits into from Jan 3, 2020

Conversation

@gloriousCode
Copy link
Collaborator

gloriousCode commented Jan 2, 2020

PR Description

I want to see where a log is coming from sometimes. I've added a new field to show which sublogger the log is coming from, but its false by default since it can be wordy.

This is what it looks like:

[DEBUG]|SYNC| 03/01/2020 07:34:34 |Binance orderbook sync complete ADA-USDT [54/150].
[DEBUG]|WEBSOCKET| 03/01/2020 07:34:35 |Binance Websocket message received: {"stream":"ltcusdt@ticker","data":{"e":"24hrTicker","E":1577997275338,"s":"LTCUSDT","p":"-2.16000000","P":"-5.176","w":"40.49841205","x":"41.72000000","c":"39.57000000","Q":"1.01343000","b":"39.54000000","B":"14.23793000","a":"39.56000000","A":"30.35120000","o":"41.73000000","h":"41.76000000","l":"39.17000000","v":"189945.05856000","q":"7692473.24805920","O":1577910874498,"C":1577997274498,"F":36409453,"L":36432844,"n":23392}}
[INFO]|TICKER| 03/01/2020 07:34:35 |Binance websocket LTC-USDT SPOT: TICKER: Last 39.57000000 Ask 39.56000000 Bid 39.54000000 High 41.76000000 Low 39.17000000 Volume 189945.05856000
[DEBUG]|WEBSOCKET| 03/01/2020 07:34:35 |Binance Websocket message received: {"stream":"ethusdt@ticker","data":{"e":"24hrTicker","E":1577997275090,"s":"ETHUSDT","p":"-4.73000000","P":"-3.583","w":"128.88328620","x":"132.02000000","c":"127.29000000","Q":"0.08902000","b":"127.27000000","B":"20.45875000","a":"127.29000000","A":"0.12970000","o":"132.02000000","h":"132.37000000","l":"126.38000000","v":"229123.11739000","q":"29530140.31345660","O":1577910873666,"C":1577997273666,"F":110951143,"L":111051893,"n":100751}}
[INFO]|TICKER| 03/01/2020 07:34:35 |Binance websocket ETH-USDT SPOT: TICKER: Last 127.29000000 Ask 127.29000000 Bid 127.27000000 High 132.37000000 Low 126.38000000 Volume 229123.11739000
[INFO]|ORDERBOOK| 03/01/2020 07:34:35 |Bithumb REST ETH-KRW SPOT: ORDERBOOK: Bids len: 30 Amount: 4833.853900 ETH. Total value: $607306.55 USD (₩704395298.35 KRW) Asks len: 30 Amount: 4894.928340 ETH. Total value: $629549.25 USD (₩730193890.29 KRW)
[DEBUG]|SYNC| 03/01/2020 07:34:35 |Bithumb orderbook sync complete ETH-KRW [55/150].
[INFO]|ORDERBOOK| 03/01/2020 07:34:35 |Bithumb REST DASH-KRW SPOT: ORDERBOOK: Bids len: 30 Amount: 452.389700 DASH. Total value: $17837.02 USD (₩20688587.12 KRW) Asks len: 30 Amount: 420.142900 DASH. Total value: $17453.24 USD (₩20243455.88 KRW)

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested

Added a new test.
Ran a benchmark on the following to help me decide how to do this:

  • go test ./... -race
  • golangci-lint run
  • TestSubLoggerName

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules
gloriousCode added 2 commits Jan 2, 2020
…ve a consistent output
@gloriousCode gloriousCode self-assigned this Jan 2, 2020
@thrasher-

This comment has been minimized.

Copy link
Collaborator

thrasher- commented Jan 2, 2020

Am a fan of adding the sub logger but not a fan of the header/spacing changes. We already give the ability for a user to customise the headers/spacing/timestamp under advanced settings in the logging section which doesn't change the defaults for everyone else

@gloriousCode gloriousCode changed the title New log output bool. Log header padding New log output bool. ~Log header padding~ Jan 2, 2020
@gloriousCode gloriousCode changed the title New log output bool. ~Log header padding~ New log output bool. ~~Log header padding~~ Jan 2, 2020
@gloriousCode gloriousCode changed the title New log output bool. ~~Log header padding~~ New log output bool Jan 2, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 2, 2020

Codecov Report

Merging #407 into master will decrease coverage by <.01%.
The diff coverage is 79.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
- Coverage   41.09%   41.08%   -0.01%     
==========================================
  Files         163      163              
  Lines       39042    39049       +7     
==========================================
+ Hits        16043    16045       +2     
- Misses      21858    21863       +5     
  Partials     1141     1141
Impacted Files Coverage Δ
logger/logger_types.go 100% <ø> (ø) ⬆️
config/config.go 85.12% <100%> (+0.02%) ⬆️
logger/logger.go 95.91% <100%> (+0.36%) ⬆️
logger/loggers.go 9.33% <25%> (ø) ⬆️
logger/logger_setup.go 59.61% <86.36%> (-4.47%) ⬇️
exchanges/bitmex/bitmex_wrapper.go 55.33% <0%> (ø) ⬆️

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 98a277a...77e647b. Read the comment docs.

Copy link
Member

xtda left a comment

Awesome feature was something I originally wanted but never got round to doing it

With the push this morning to remove the omitempty can we also get it added to config_example.json as its a handy feature to let users know of anyway?

InfoHeader: c.AdvancedSettings.Headers.Info,
WarnHeader: c.AdvancedSettings.Headers.Warn,
DebugHeader: c.AdvancedSettings.Headers.Debug,
ShowLogSystemName: *c.AdvancedSettings.ShowLogSystemName,

This comment has been minimized.

Copy link
@xtda

xtda Jan 2, 2020

Member

This will be a nil pointer and cause a segfault if people don't have "showLogSystemName" in their config

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 2, 2020

Author Collaborator

Whoopsies!

RESTSys = registerNewSubLogger("REST")

Ticker = registerNewSubLogger("TICKER")
OrderBook = registerNewSubLogger("ORDERBOOK")

This comment has been minimized.

Copy link
@xtda

xtda Jan 2, 2020

Member

While I don't really mind that these are changed to upper case can we either add support for migrating existing to upper case or even something as simple as strings.ToUpper in registerNewSubLogger

(I have a config setup already with some subloggers configured :D)

Copy link

codelingo bot left a comment

LGTM

Copy link

codelingo bot left a comment

LGTM

Copy link
Collaborator

thrasher- left a comment

Thanks for making those requested changes! Just one HUGE nit
image

After that, it LGTM

config/config.go Show resolved Hide resolved
@thrasher- thrasher- changed the title New log output bool Logger: Add support for toggling sub logger name output for log events Jan 3, 2020
Copy link
Collaborator

thrasher- left a comment

tACK! 💯

}

logger.ShowLogSystemName = false
w = &bytes.Buffer{}

This comment has been minimized.

Copy link
@xtda

xtda Jan 3, 2020

Member

this can be changed to w.Reset()

Copy link
Collaborator

shazbert left a comment

tACK - Fine Work!

@xtda
xtda approved these changes Jan 3, 2020
Copy link
Member

xtda left a comment

thanks for making the changes!
tested approved 🌮

@thrasher- thrasher- merged commit f8ef6da into thrasher-corp:master Jan 3, 2020
4 of 5 checks passed
4 of 5 checks passed
Review Action Errored whilst reviewing.
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 79.06% of diff hit (target 41.09%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +37.97% compared to 98a277a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.