[log] better trace log #3914

millken · 2023-08-03T05:48:07Z

Description

Due to the requirement of structured JSON logs for Signoz log collection, the current format of console logs cannot be parsed by Signoz. Additionally, the logs do not contain a trace_id, making it difficult to combine tracing and logs effectively.

This PR addresses these issues by implementing the following changes:

Disable logging in automaxprocs that is not in JSON format.
Wrap the zap.Logger to print the trace_id when there is a api trace log. This requires using log.Ctx(ctx), these logs can be sent to signoz by configuring whether.

Fix the issue where JSON encoded logs would still forcefully output console logs.

Fixes #(issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[] make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] 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 with my changes
[] Any dependent changes have been merged and published in downstream modules

codecov · 2023-08-03T09:56:19Z

Codecov Report

Merging #3914 (2571d1c) into master (e1f0636) will increase coverage by 0.78%.
Report is 93 commits behind head on master.
The diff coverage is 77.14%.

❗ Current head 2571d1c differs from pull request most recent head 8aae37f. Consider uploading reports for the commit 8aae37f to get more accurate results

@@            Coverage Diff             @@
##           master    #3914      +/-   ##
==========================================
+ Coverage   75.38%   76.16%   +0.78%     
==========================================
  Files         303      328      +25     
  Lines       25923    27971    +2048     
==========================================
+ Hits        19541    21303    +1762     
- Misses       5360     5573     +213     
- Partials     1022     1095      +73

Files	Coverage Δ
action/candidate_register.go	`90.00% <100.00%> (ø)`
action/candidate_update.go	`89.01% <100.00%> (+0.12%)`	⬆️
action/claimreward.go	`90.62% <100.00%> (-0.42%)`	⬇️
action/depositreward.go	`87.50% <100.00%> (-0.56%)`	⬇️
action/protocol/context.go	`67.91% <100.00%> (+0.73%)`	⬆️
action/protocol/execution/evm/evmstatedbadapter.go	`66.77% <ø> (ø)`
action/protocol/execution/protocol.go	`42.10% <100.00%> (ø)`
action/protocol/rewarding/reward.go	`90.07% <100.00%> (+0.60%)`	⬆️
...tion/protocol/staking/candidate_buckets_indexer.go	`89.31% <100.00%> (-0.47%)`	⬇️
...col/staking/ethabi/stake_composite_bucketscount.go	`100.00% <100.00%> (ø)`
... and 67 more

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

dustinxie · 2023-08-04T06:02:18Z

.github/workflows/ci.yaml

@@ -25,7 +25,7 @@ jobs:
      - name: Set up Go
        uses: actions/setup-go@v3
        with:
-          go-version: 1.18.5


revert if this is not required

after updating go.mod, need to upgrade to go.1.19

# go.uber.org/multierr Error: ../../../go/pkg/mod/go.uber.org/[email protected]/error.go:209:20: undefined: atomic.Bool note: module requires Go 1.19 # go.opentelemetry.io/otel/sdk/trace Error: ../../../go/pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/provider.go:78:24: undefined: atomic.Pointer Error: ../../../go/pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/provider.go:80:20: undefined: atomic.Bool note: module requires Go 1.19 Error: Process completed with exit code 2.

dustinxie · 2023-08-04T06:02:24Z

.github/workflows/release.yaml

@@ -28,7 +28,7 @@ jobs:
      - name: Set up Go
        uses: actions/setup-go@v2
        with:
-          go-version: 1.18.5


revert if this is not required

dustinxie · 2023-08-04T06:05:15Z

api/http.go

@@ -64,14 +63,12 @@ func newHTTPHandler(web3Handler Web3Handler) *hTTPHandler {
 }

 func (handler *hTTPHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
-	ctx, span := tracer.NewSpan(req.Context(), "handler.ServeHTTP")
-	defer span.End()


there are other places calling tracer.NewSpan(), should same changes be applied there as well?

Yes, can remove this later and use log.Ctx instead. But it can be used when there are special needs (such as funcs spent time)

actually log.Ctxis an event in the span

envestcc · 2023-08-04T13:50:20Z

pkg/log/tracelog.go

+)
+
+// Ctx is a wrapper of otelzap.Ctx, returns a logger with context.
+func Ctx(ctx context.Context) otelzap.LoggerWithCtx {


The name Ctx may be misleading. Using T(ctx) may be a better alternative as it denotes tracing, and is consistent with the L() and S() methods in log pkg.

sonarqubecloud · 2023-08-07T03:45:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

pkg/log/log.go

Liuhaai · 2023-08-10T00:18:12Z

pkg/log/log.go

@@ -115,6 +126,9 @@ func InitLoggers(globalCfg GlobalConfig, subCfgs map[string]GlobalConfig, opts .
 				zap.RedirectStdLog(logger)
 			}
 			zap.ReplaceGlobals(logger)
+			if err := initTraceLogger(logger, cfg.Trace); err != nil {


why is it initiated here?

trace logger needs to wrap a global zap logger, here is global zap logger initiated

Liuhaai · 2023-08-10T00:24:37Z

pkg/log/tracelog.go

+)
+
+// T is a wrapper of otelzap.Ctx, returns a logger with context.
+func T(ctx context.Context) otelzap.LoggerWithCtx {


listen logs from L(), etc.

after initTraceLogger, T(ctx) and L() use the same zap logger.

In different , T(ctx) will additionally attach some trace fields and send them to the trace provider(jaeger/signoz)

ok. Does the tracer work as before with this pr?

Liuhaai · 2023-08-10T00:27:10Z

server/main.go

@@ -73,6 +73,8 @@ func init() {
 }

 func main() {
+	// set max number of CPUs, disable log printing
+	maxprocs.Set(maxprocs.Logger(nil))


only for json format?

yes, Its log is not controlled by zap and will break the format of zap json log

this line will be executed even if for console format

sonarqubecloud · 2023-09-08T03:39:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sonarqubecloud · 2023-11-03T00:51:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

better trace log

427cc5a

millken requested review from CoderZhi, dustinxie, Liuhaai and a team as code owners August 3, 2023 05:48

upgrade go version(ci)

c0ec2c8

millken requested a review from xianhuawei as a code owner August 3, 2023 08:43

millken added 3 commits August 3, 2023 17:05

fix go.mod

e54686b

fix go.mod

51f9fc0

fix go.mod

ba6b835

xianhuawei approved these changes Aug 3, 2023

View reviewed changes

dustinxie reviewed Aug 4, 2023

View reviewed changes

envestcc reviewed Aug 4, 2023

View reviewed changes

envestcc approved these changes Aug 4, 2023

View reviewed changes

millken added 2 commits August 5, 2023 16:23

rename log.Ctx to log.T

1617038

Merge branch 'master' into better-trace

66bb28f

Liuhaai reviewed Aug 10, 2023

View reviewed changes

pkg/log/log.go Show resolved Hide resolved

Liuhaai reviewed Aug 10, 2023

View reviewed changes

Merge branch 'master' into better-trace

b92983b

millken added 2 commits November 2, 2023 15:00

Merge branch 'master' into better-trace

8505962

move maxprocs to init func

3babdae

dustinxie approved these changes Nov 2, 2023

View reviewed changes

Merge branch 'master' into better-trace

8aae37f

millken merged commit 32cbd3c into iotexproject:master Nov 3, 2023
3 checks passed

dustinxie mentioned this pull request Nov 16, 2023

fix Dockerfile #3970

Merged

10 tasks

BrewTestBot mentioned this pull request Dec 22, 2023

ioctl 1.12.1 Homebrew/homebrew-core#158048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[log] better trace log #3914

[log] better trace log #3914

millken commented Aug 3, 2023 •

edited

Loading

codecov bot commented Aug 3, 2023 •

edited

Loading

dustinxie Aug 4, 2023

millken Aug 4, 2023

dustinxie Aug 4, 2023

dustinxie Aug 4, 2023

millken Aug 4, 2023

envestcc Aug 4, 2023

sonarqubecloud bot commented Aug 7, 2023

Liuhaai Aug 10, 2023

millken Aug 10, 2023

Liuhaai Aug 10, 2023 •

edited

Loading

millken Aug 10, 2023 •

edited

Loading

Liuhaai Aug 10, 2023

Liuhaai Aug 10, 2023 •

edited

Loading

millken Aug 10, 2023

Liuhaai Aug 10, 2023

sonarqubecloud bot commented Sep 8, 2023

sonarqubecloud bot commented Nov 3, 2023

[log] better trace log #3914

[log] better trace log #3914

Conversation

millken commented Aug 3, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Aug 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liuhaai Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

millken Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liuhaai Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 8, 2023

sonarqubecloud bot commented Nov 3, 2023

millken commented Aug 3, 2023 •

edited

Loading

codecov bot commented Aug 3, 2023 •

edited

Loading

Liuhaai Aug 10, 2023 •

edited

Loading

millken Aug 10, 2023 •

edited

Loading

Liuhaai Aug 10, 2023 •

edited

Loading