newrelic / go-agent

New Relic Go Agent
Apache License 2.0
775 stars 296 forks source link

nrzap log in context ignores additional fields #768

Closed NicholasGWK closed 6 months ago

NicholasGWK commented 1 year ago

Description

I'm unsure if this is intended behaviour or not, but while debugging I noticed the nrzap logs in context integration does NOT do anything with zap fields...my initial expectation was that these would be added as custom attributes to the log entries.

The code here shows that the fields are just not used: https://github.com/newrelic/go-agent/blob/046b4fc116adac2b4dec4d691b61a25b80488fc2/v3/integrations/logcontext-v2/nrzap/nrzap.go#L28

Please let me know if this intended or if there's another way to add these fields as attributes!

Steps to Reproduce

txnLogger.Error("Some error!")

do

txnLogger.Error("Some error!", zap.Int("http.statusCode", http.StatusInternalServerError)

Expected Behavior

The log entry in New Relic would have a custom attributes, "http.statusCode" with the passed value.

I didn't include env info because I'm not sure it matters based on the code I linked to, but please let me know if any additional info is needed!

Thanks so much,

Nick

stefanoschrs commented 1 year ago

Do we have any updates on this?

Adding some fields to both logs in the example doesn't work for me either as intended:

    backgroundLogger.Info("this is a background log message", zap.String("foo", "bar"))
...
    txnLogger.Info("this is a transaction log message", zap.String("foo", "bar"))

Gives this in the console:

{"level":"info","ts":1695784536.173117,"msg":"this is a background log message","foo":"bar"}
{"level":"info","ts":1695784536.173432,"msg":"this is a transaction log message","foo":"bar"}

but in New Relic:

{
  "entity.guid": "xxx",
  "entity.guids": "xxx",
  "entity.name": "xxx",
  "hostname": "xxx",
  "level": "info",
  "message": "this is a background log message",
  "newrelic.source": "xxx",
  "timestamp": xxx
}

{
  "entity.guid": "xxx",
  "entity.guids": "xxx",
  "entity.name": "xxx",
  "hostname": "xxx",
  "level": "info",
  "message": "this is a transaction log message",
  "newrelic.source": "xxx",
  "span.id": "xxx",
  "timestamp": xxx,
  "trace.id": "xxx"
}
iamemilio commented 1 year ago

Yeah, we are aware of this. There is a roadmap item to add support for logs in context fields in the future, and a possible re-write of the zap libraries that will pair with it. The collection of fields as attributes is not supported in the agent yet.

stefanoschrs commented 1 year ago

Thanks for the reply, is there a very rough estimation on when will this happen? Just to know if it's worth waiting or switching to the REST agent way.

adomaskizogian commented 7 months ago

bumping this just run into the same issue with slog.

it is sad that such core functionality is missing and forces us to do parsing of logs on ui

@iamemilio I don't believe this should be labeled as an enhancement. It is rather a bug. It is a core functionality that simply does not work. Either this or the logcontext-v2 lib MVP scope was poorly defined and it should not had been made public. I can't imagine anyone making a decision to not support attribute capture and call it a properly implemented library in 2024 which is authored by observability platform engineers. If anything, new relic should be driving the observability practices and application to next level, not to bring it down to unstructured logging.

NewRelic go-agent only recently started supporting more than logrus and yet it fails to do it right. Despite that the only option remains to use the plugin coming from logcontext v1 with logrus cause it supports attribute capture. So why even bother implementing the new integration labraries if none of them support core must have functionality. This forces customers to parse unstructured log data using regexp and nrql to make anything useful with dashboards/alerts/insights and/or locks in with logrus which is in maintenance mode only.

iamemilio commented 7 months ago

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 here and in #833.

iamemilio commented 6 months ago

I am working on adding support for attribute collection to the agent now. Once I have that code settled and tested, I will prioritize Zap and then Slog for attribute collection support first. Zerolog's opacity will make attribute collection more difficult, so I'll save it for last.

iamemilio commented 6 months ago

Entering the review phase for attribute collection support in the agent: https://github.com/newrelic/go-agent/pull/900. Building this into wrappers for logging frameworks and additional optimizations are on their way. This should make the next release. Feel free to leave feedback on this PR until then.

Notes:

mirackara commented 6 months ago

902 is the WIP PR specifically for zap attribute collection. We are currently in the testing phase for attribute collection for zap. Along with #900, this should also be in the next release. Feel free to leave feedback/suggestions on this PR as well!

iamemilio commented 6 months ago

This feature has been released for Zap in the 3.33.0 version of the agent! https://github.com/newrelic/go-agent/pull/908