newrelic / newrelic-dotnet-agent

The New Relic .NET language agent.
Apache License 2.0
98 stars 62 forks source link

Serilog Logs in context from separated NR_LINKING property (instead of force Message to be changed) #2178

Closed mikeloguay closed 9 months ago

mikeloguay commented 11 months ago

Is your feature request related to a problem? Please describe.

Make Logs in context feature work with Serilog is far from ideal in my opinion, since the system requires that NR_LINKING metadata is included as part of the log event Message (https://docs.newrelic.com/docs/logs/logs-context/net-configure-logs-context-all/#2-decorate)

Feature Description

I think the .NET agent (or maybe another internal NewRelic component) should be able to read the metadata information from a separated NR_LINKING property, without the need of render all the content inside the Message itself.

Describe Alternatives

At the moment, we are using Serilog with a custom fluentd sink (mainly a fork of https://github.com/klyse/serilog-sinks-fluentd). We are generating JSON messages that come to fluentd and then they are forwarded to New Relic. I needed to change the code of the sink, changing the record before emitted to add this information. This adds complexity and a performance penalty (~x2 in time as per my basic calculations).

The code involved on change the record before emit is something similar to this:

string formattedLinkingData = logEvent.Properties[NR_LINKING_PROPERTY_KEY].ToString().Replace("\"", "");
var record = new Dictionary<string, object>
{
    {"level", logEvent.Level.ToString()},
    {options.MessageTemplateKey, logEvent.MessageTemplate.Text},
    {options.MessageKey, $"{formattedLinkingData}{logEvent.MessageTemplate.Render(logEvent.Properties, options.FormatProvider)}"}
};

As per my investigation ToString() is the responsible of the performance problem. With an automatic management inside the agent, none of this could would be needed.

Priority

For me this is a "Really Want" priority, since I would like to have Logs in context feature working OK.

Do you guys think this is feasible?

Many thanks in advance.

workato-integration[bot] commented 11 months ago

https://new-relic.atlassian.net/browse/NR-213002

tippmar-nr commented 10 months ago

@mikeloguay Thanks for your submission. Unfortunately, we are constrained by the New Relic endpoints where log data is sent; those endpoints require the NR_LINKING property to be present in the log message itself. We will keep your feature request on our backlog and product management will determine whether it's something we are able to work on.

mikeloguay commented 10 months ago

Hi @tippmar-nr , thanks for your quick response.

If the requirement is somewhere else (NR endpoints as you said), then maybe you can create an issue in the corresponding repository, or link this issue with another one, if you consider useful for tracking.

In the other hand, any help to improve the performance of my workaround would be appreciated as well, meanwhile you guys consider this issue to be addressed.

Thanks.

nrcventura commented 10 months ago

As an alternative, you could disable the automatic agent log decoration, and use https://github.com/newrelic/newrelic-logenricher-dotnet/tree/main/src/NewRelic.LogEnrichers.Serilog instead. The New Relic Serilog log enricher library will also add the necessary data to your logs so that the logs in context feature will be available. Both the log enrichers and Agent automatic log decoration add the same data to the logs, but use different formats.

The logging teams use a different tool for tracking issues so we are unable to link this issue in another public location (but this issue is referenced internally). This information was passed along to Product Management so that they can have the necessary discussions and prioritize things accordingly.

mikeloguay commented 10 months ago

Hi @nrcventura,

Thanks for your reply. I am already doing this way, with automatic decorating disabled, and using NR logs enricher. Here you have an extraction of my config file:

"Enrich": [
  "WithMachineName",
  "WithThreadId",
  "WithThreadName",
  "WithNewRelicLogsInContext"
],

So the NR-LINKING info is correctly added, but as separated fields, not changing at all the Message property. This change needs to be done manually by me in the code of our sink, and that is the purpose of this issue.

Thanks.

nrcventura commented 10 months ago

I agree that it would be a smoother experience if something similar could have been done automatically by the agent. Unfortunately, I do not understand why that approach was not supported by the other teams we have a dependency on.

workato-integration[bot] commented 9 months ago

The issue will not be fixed.