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

Added span and trace id to hotrod logs #2384

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

joe-elliott
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • Adds trace ID and span ID hotrod logs as requested.

Example:

2020-08-11T22:20:15.010-0400	INFO	route/client.go:54	Finding route	{"service": "frontend", "component": "route_client", "trace_id": "6b1ab6a4c91f3780", "span_id": "6b1ab6a4c91f3780", "pickup": "193,499", "dropoff": "577,322"}
2020-08-11T22:20:15.010-0400	INFO	route/client.go:54	Finding route	{"service": "frontend", "component": "route_client", "trace_id": "6b1ab6a4c91f3780", "span_id": "6b1ab6a4c91f3780", "pickup": "476,860", "dropoff": "577,322"}
2020-08-11T22:20:15.010-0400	INFO	route/client.go:54	Finding route	{"service": "frontend", "component": "route_client", "trace_id": "6b1ab6a4c91f3780", "span_id": "6b1ab6a4c91f3780", "pickup": "324,354", "dropoff": "577,322"}
2020-08-11T22:20:15.010-0400	INFO	route/server.go:71	HTTP request received	{"service": "route", "trace_id": "6b1ab6a4c91f3780", "span_id": "4a17e4c6aeb157ad", "method": "GET", "url": "/route?dropoff=577%2C322&pickup=193%2C499"}
2020-08-11T22:20:15.011-0400	INFO	route/server.go:71	HTTP request received	{"service": "route", "trace_id": "6b1ab6a4c91f3780", "span_id": "3b22d1cc6f90e8c", "method": "GET", "url": "/route?dropoff=577%2C322&pickup=476%2C860"}
2020-08-11T22:20:15.011-0400	INFO	route/server.go:71	HTTP request received	{"service": "route", "trace_id": "6b1ab6a4c91f3780", "span_id": "12de05f88735056f", "method": "GET", "url": "/route?dropoff=577%2C322&pickup=324%2C354"}

I know this was assigned to @ghostsquad, but it's been over a month so I took a stab at it.

@joe-elliott joe-elliott requested a review from a team as a code owner August 12, 2020 02:20
@ghostsquad
Copy link

oooh, my bad, ya I've been busy with other things! sorry about that.

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #2384 into master will increase coverage by 0.00%.
The diff coverage is 78.26%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2384   +/-   ##
=======================================
  Coverage   95.61%   95.61%           
=======================================
  Files         205      206    +1     
  Lines       10529    10547   +18     
=======================================
+ Hits        10067    10085   +18     
+ Misses        395      394    -1     
- Partials       67       68    +1     
Impacted Files Coverage Δ
...el/converter/thrift/jaeger/sampling_from_domain.go 100.00% <ø> (ø)
cmd/agent/app/builder.go 96.00% <25.00%> (-4.00%) ⬇️
cmd/agent/app/servers/thriftudp/socket_buffer.go 60.00% <60.00%> (ø)
cmd/agent/app/flags.go 93.33% <100.00%> (+0.47%) ⬆️
cmd/agent/app/servers/tbuffered_server.go 100.00% <100.00%> (ø)
cmd/agent/app/servers/thriftudp/transport.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/flags.go 100.00% <100.00%> (ø)
plugin/storage/kafka/options.go 94.05% <100.00%> (+0.37%) ⬆️
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️
... and 2 more

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 6e04d9d...6624f59. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm.

Internally at Uber we have an extension to zap.Logger that supports the notion of context extractors, e.g.

type ContextFieldExtractor = func(context.Context) zapcore.Field

and is configured with the extractor defined in Jaeger client "github.com/uber/jaeger-client-go/log/zap".Trace 

Then instead of For(ctx) and a custom logger as we do here users can use normal zap.Logger with a field Context(ctx), defined as below:

const (
   // n.b. This is used in conjunction with the skip field produced by Context() to indicate that
   //      this is a special-cased field, enabling zapfx to detect context fields and subsequently
   //      run any configured ContextFieldExtractors. When used, the corresponding zapcore.Field
   //      looks similar to:
   //
   //          zapcore.Field{
   //            Type:      zapcore.SkipType,
   //            Integer:   _contextFieldIndicator,
   //            Interface: ctx,
   //          }
   _contextFieldIndicator int64 = 1<<8 - 1
 )
    
   // Context wraps ctx as a zapcore.Field, suitable for use with calls to zap.Logger. It may only be
   // used by loggers provided by zapfx; other loggers will panic due to its internal field type.
   //
   // Importantly, Context differs from other zapcore.Field types in that it does not guarantee a 1:1
   // ratio of provided:encoded fields. Specifically, it:
   //
   //   - May not result in an encoded field, in cases in which there are no configured underlying
   //     context extractors or where context baggage does not contain the necessary context keys;
   //   - May result in more than one encoded field, in cases in which multiple context extractors
   //     find keys and produce fields; and
   //   - Does not indicate a concrete field key or value at log time, as key:value pairs of any
   //     resulting field(s) are not known until the configured extractors have run just prior to log
   //     entry encoding.
 func Context(ctx context.Context) zapcore.Field {
     return zapcore.Field{
       Type:      zapcore.SkipType,
       Integer:   _contextFieldIndicator,
       Interface: ctx,
     }
 }

Unfortunately, it never made it to OSS.

@yurishkuro yurishkuro merged commit 6f8fe44 into jaegertracing:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TraceID to Logs in HotRod Example Application
3 participants