-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Newrelic Logs Integration: Unnest messages/named_tags, leverage nested objects, quick hacky Dockerfile #307
base: master
Are you sure you want to change the base?
Conversation
86f4f78
to
a97cebc
Compare
33552d6
to
2efb22a
Compare
Hope you're gearing up for an excellent New Years! If you have some time to review this, I'll be happy to do what's needed to get it across the finish line. Thanks for your work on the project! |
Yes please, would love to avoid the extra nesting under |
I just deployed this on my end and I believe this is now working as most will expect but additional testing is certainly appreciated! One issue I'm still seeing: It doesn't look like span.id or trace.is being included as expected (though that was also the case prior to my changes) Is anyone else seeing that behavior? |
@rtizzy I tested your PR in my project and it work like a charm 🚀 I'm eager to see this in production hope it will be merged soon. Just one quick possible improvement, since NewRelic logs analysis dashboard expects a |
@reidmorrison any chance to have this merged? |
There still seem to be at least a few issues with this implementation. See the following code as an example
The intent is that should have a few unnamed tags nested under tags and then a few named tags for What I'm actually seeing is that it continues to nest everything under the If anyone has any suggestions for a fix, I'd appreciate it. Don't currently have the time to dig into it. :( |
I don't think that's your patch, but is the normal behavior of SemanticLogger, see this pry session (no JSON, no NewRelic):
If you want to have both array tags and hash tags you need to perform two different calls as in:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello from New Relic! 👋
Thanks for putting this together @rtizzy! I wanted to check in on the comments about linking metadata. What's the latest status on span_id
, trace_id
, entity_name
, etc? Is the agent attaching them to the event?
The comment in the formatters/new_relic_logs.rb
file and the passing tests, make it seem like at least trace_id
and span_id
are working, but @fabn's comment got me especially curious about the service linking metadata (ex. entity_name
).
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Thanks for reaching out. Hope you're well.
Here is an example redacted log message that I'm receiving, some of these, like I would expect things like How does that associated happen and is there a good way to "force it" to check that it's working?
|
Issue # (if available)
#300
Changelog
Changelog updated appropriately.
Content below
Description of changes
This PR fixes the NR Log Formatter so that keys are no longer erroneously nested under the
message.
key as mentioned in the linked issue.This was initially commented on in the original PR but was simply missed.
messages
named_tag_conflicts
key so they are clearly visible and can be alerted on via NR.Screenshots
Before
After
Other details
Newrelic documentation is often incredibly contradictory with things spread all over the place.
Here they mention that the addition of a
logtype
value is required to utilize any log parsing rules in their best practices guidehttps://docs.newrelic.com/docs/logs/get-started/logging-best-practices/
They then contradict themselves in the Log Parsing docs saying that while this is suggested, it is not explicitly required.
https://docs.newrelic.com/docs/logs/ui-data/parsing/
I can find no example for adding this via the Ruby APM Agent in their docs. I believe it may be possible to do so with the following configuration variable but I'm uncertain if this decoration will work with the SL gem.
https://docs.newrelic.com/docs/apm/agents/ruby-agent/configuration/ruby-agent-configuration/#application_logging-forwarding-custom_attributes
If we find that adding this at the APM level does not append this to the logs, another PR should likely be created to ensure it's possible to optionally set this value (Possibly with a default value like "logtype: semanticlogger" or similar).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.