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

Attributes are not forwarded to NewRelic for logs #883

Open
mariusgrigaitis opened this issue Mar 27, 2024 · 24 comments
Open

Attributes are not forwarded to NewRelic for logs #883

mariusgrigaitis opened this issue Mar 27, 2024 · 24 comments
Assignees
Labels
bug p1 Highest priority work items roadmap

Comments

@mariusgrigaitis
Copy link

mariusgrigaitis commented Mar 27, 2024

Logs in context integrations and newrelic-agent does not forward attributes from logs

https://docs.newrelic.com/docs/logs/logs-context/configure-logs-context-go/#installing-a-logs-in-context-plugin

https://github.com/newrelic/go-agent/blob/master/v3/integrations/logcontext-v2/nrslog/handler.go#L130

Expected Behavior

Attributes appear on NR.

Reproduction case

https://github.com/newrelic/go-agent/blob/master/v3/integrations/logcontext-v2/nrslog/example/main.go

Additional attributes can be added to prove the case in the linked example

Additional context

What's the point of integrating structured logging solutions into NewRelic if attributes (essential part of structured) are not being forwarded?

Seems to be the case for all logcontext-v2 integrations, since attributes are not part of logcontext-v2 design?

Related: #768

@r--w
Copy link

r--w commented Apr 2, 2024

This functionality is quite critical. Good slog practice requires adding attributes with additional metadatada, right now all is lost, only message is displayed in NewRelic's console.

@r--w
Copy link

r--w commented Apr 3, 2024

@mariusgrigaitis I managed to add a simple middleware to add all the attributes to the message and this way you can at least see meaningful logs in NewRelic's console, here's the part of the code using https://github.com/samber/slog-multi/

....
logger := slog.New(
		slogmulti.
			Pipe(slogmulti.NewHandleInlineMiddleware(concatAttrsMiddleware)).
			Handler(nrslog.JSONHandler(app, os.Stdout, opts)),
	)

....
func concatAttrsMiddleware(ctx context.Context, record slog.Record, next func(context.Context, slog.Record) error) error {
	flat := strings.Builder{}

	record.Attrs(func(attr slog.Attr) bool {
		flat.WriteString("key: " + attr.Key + " value: " + fmt.Sprintf("%v", attr.Value))
		return true
	})

	message := record.Message
	if flat.Len() > 0 {
		message += ", " + flat.String()
	}

	return next(ctx, slog.NewRecord(record.Time, record.Level, message, record.PC))
}

@iamemilio
Copy link
Contributor

iamemilio commented Apr 5, 2024

Hi all, thank you for your advocacy and your patience. I hear your concerns and have gotten the go ahead to prioritize this ASAP. Supporting attribute collection for all v2 logs in context packages is my top priority. I will communicate expectations and upcoming changes as they come. I will communicate about this primarily in #768 since that is where most people seem to be tracking this issue.

@iamemilio iamemilio self-assigned this Apr 8, 2024
@iamemilio iamemilio added roadmap p1 Highest priority work items labels Apr 8, 2024
@clarkmcc
Copy link

clarkmcc commented Apr 8, 2024

Thank you! I fought with this for hours before I realized it was expected behavior. Looking forward to a fix.

@miyamo2
Copy link

miyamo2 commented Apr 13, 2024

I am also developing on a library as a hobby to include attributes in the Logs in Context while also logging to stdout in JSON format.

I hope you can use this as a temporary solution.

https://github.com/miyamo2/altnrslog

@iamemilio
Copy link
Contributor

Capturing attributes should already work for decorated logs captured by log forwarders, but the agent is not yet able to handle and emit log attributes. We will have that code settled and shipped soon, and then we can start plugging it into our logging tools: #900

@iamemilio
Copy link
Contributor

#768 (comment)

@iamemilio
Copy link
Contributor

https://github.com/newrelic/go-agent/releases/tag/v3.33.0 - Adds this feature to Zap.
Upcoming releases will add this to zerolog and slog too

@mirackara
Copy link
Contributor

#911 PR for slog attribute support.

@dselans
Copy link

dselans commented May 26, 2024

Same as @clarkmcc , spent hours troubleshooting this until I found this issue.

Using logrus with .WithField() and .WithFields().

@conamu
Copy link

conamu commented Jun 3, 2024

#911 PR for slog attribute support.

I saw it was approved a month ago. Any updates on when this is going to get released?

@dselans
Copy link

dselans commented Jun 3, 2024

@conamu They added support for attributes for uber/zap in 3.33.0 - it works well but it doesn't include initial fields or fields added via WithField() :(

As a tmp bandaid, I added my own log wrapper that keeps track of the fields and just re-adds them every time a log.Debug/Info/Warn/etc is called (with or without extra attributes). Could do the same for logrus/whatever other logger you're using.

Gist: https://gist.github.com/dselans/b318d89158ad83b930221650db15c6ec

@conamu
Copy link

conamu commented Jun 3, 2024

we are already using logrus in most of the codebase. Im more interested in a solution to work with slog, a native package, instead of having to fix another library.

@iamemilio
Copy link
Contributor

@iamemilio
Copy link
Contributor

Working on a fix for #915

@iamemilio
Copy link
Contributor

See: #919

@BoldBrandonM
Copy link

Keenly looking forward to the slog attribute handling. I could incorporate a workaround but I would rather not include additional libraries to intercept the slog output.

Any word on when this is planned to be included in a release now that it's merged?

@iamemilio
Copy link
Contributor

iamemilio commented Jul 30, 2024

We are hoping to do a release this Thursday. These logging features will be included in that.

@BoldBrandonM
Copy link

Fantastic news, thank you!

@iamemilio
Copy link
Contributor

We are deferring the release to Monday August 5th to make space to address #939, which is a high priority issue. Thanks for your patience, we are eager to get these features in your hands!

@iamemilio
Copy link
Contributor

iamemilio commented Aug 5, 2024

As of the release 3.34.0, the Go agent should support full attribute collection for Zap, Slog, and Logrus. Support for Zerolog is on the way in an upcoming release!

@BoldBrandonM
Copy link

As of the release 3.34.0, the Go agent should support full attribute collection for Zap, Slog, and Logrus. Support for Zerolog is on the way in an upcoming release!

Thank you! Just tested this out with Slog and it's working well for our use-case 🥂

@conamu
Copy link

conamu commented Nov 26, 2024

Any updates on this? Im using go agent v3.35.1 and it is still not forwarding custom attributes.
The only attribute I saw that is added is "error" and all other ones are dropped.

Maybe also related to #986 since im also using slog.With()

How I set up my logger:

handler := nrslog.TextHandler(app, os.Stdout, &slog.HandlerOptions{
		Level: slog.SetLogLoggerLevel(parseLogLevel()),
	})

	log := slog.New(nrslog.WithTransactionFromContext(handler))
	log = log.With("env", env)
log := slog.New(nrslog.TextHandler(app, os.Stdout, &slog.HandlerOptions{
		Level: slog.SetLogLoggerLevel(parseLogLevel()),
	}))
	log = log.With("env", env)

both ways are not sending attributes with logs.

@conamu
Copy link

conamu commented Nov 26, 2024

@iamemilio /|\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug p1 Highest priority work items roadmap
Projects
None yet
Development

No branches or pull requests

9 participants