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

Make it possible to extract fields from the context in the slog adapter #1372

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

recht
Copy link

@recht recht commented Oct 9, 2023

Since slog has context logging built in it would be nice to be able to utilize that with a zap handler, for example to log tracing context.

Since slog has context logging built in it would be nice to be able to utilize that with a zap handler, for example to log tracing context.
@CLAassistant
Copy link

CLAassistant commented Oct 9, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@r-hang r-hang left a comment

Choose a reason for hiding this comment

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

This makes sense to me! I think the name WithFieldsFromContext is a bit more descriptive than WithContextExtractor.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (87577d8) 98.28% compared to head (a241cd5) 98.43%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1372      +/-   ##
==========================================
+ Coverage   98.28%   98.43%   +0.14%     
==========================================
  Files          53       53              
  Lines        3493     3505      +12     
==========================================
+ Hits         3433     3450      +17     
+ Misses         50       46       -4     
+ Partials       10        9       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@recht
Copy link
Author

recht commented Oct 24, 2023

This makes sense to me! I think the name WithFieldsFromContext is a bit more descriptive than WithContextExtractor.

I can change it to that, but do you then have any suggestion for the field/type name?

@r-hang
Copy link
Contributor

r-hang commented Oct 24, 2023

My apologies, I've thought about it some more and looked at more code and propose a modification to

WithContextFieldExtractors(extractors ...ContextFieldExtractor), where a ContextFieldExtractor takes a context.Context and returns a single zapcore.Field.

I think this will make composing and sharing logic between multiple extractors a bit easier than if the option constructor took in a single extractor function that returned multiple fields. What do you think?

@recht
Copy link
Author

recht commented Dec 7, 2023

Sorry for the late reply - we could do that instead, but it will lead to some amount of duplicated work. My specific use case is to add tracing info to logs, and for that I have something like this:

zapslog.WithContextExtractor(func(ctx context.Context) []zapcore.Field {
		span := trace.SpanFromContext(ctx)
		sctx := span.SpanContext()
		if sctx.IsValid() {
			return []zapcore.Field{
				zap.Stringer("trace_id", sctx.TraceID()),
				zap.Stringer("span_id", sctx.SpanID()),
				zap.Int8("trace_flags", int8(sctx.TraceFlags())),
			}
		}
		return nil
	})

Splitting that into 3 extractors instead then requires me to get the span context 3 times, check if it's valid, and then emit single fields.

How about instead allowing multiple extractors, but allow them to still return a slice of fields?

@r-hang
Copy link
Contributor

r-hang commented Dec 11, 2023

Sorry for the delay, I will follow up here by Weds!

r-hang
r-hang previously approved these changes Dec 12, 2023
Copy link
Contributor

@r-hang r-hang left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

Providing options for multiple extractors that can provide multiple fields seems adaptable to both cases where someone might want to re-use context extractors or provide a single one for all of their fields.

Will approve after CI passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants