-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add log.logger #521
Add log.logger #521
Conversation
This reminds me off https://github.com/elastic/beats/blob/master/filebeat/module/elasticsearch/_meta/fields.yml#L10 which we use in the Elasticsearch logs. Do you see this as specific to Java or more general? Perhaps |
I'm not entirely sure whether logging implementations in other languages tend to have name for a logger which is usually scoped to one class. @elastic/apm-agent-devs do you know? But in general, it's not specific to Java. Even if it turns out only Java loggers tend to use it I think it makes sense to have this as a general field. |
If we tie it to Logging but not Java, what about |
that would also work for me |
Currently +1 on |
Logger-per-class pattern is definitely not something specific to Java. But I don't know what are the prevalent logging approaches in languages without class concept. |
|
simple, clean and straight to the point. +1 |
This reverts commit 8176d54.
Another +1 for |
I can also confirm that the logging class/component can be a thing in Ruby as well. I wonder if we shouldn't take a step back and think about this more holistically, though. Even in normal logs, you can have the function/method name provided by some loggers (e.g. Rails' However, the more we dig into these details, the less it makes sense to cram all this in Should we look into putting together a more complete application logging / debugging field set? This field set could encompass the source code details as I'm discussing here, as well as optionally exception details, if the event is about an exception. Let's use
I'm excluding I'm sure there's a few more attributes we could add, here. This is just a quick first draft. |
Good point but I think linking to source code is a separate beast and we already have an issue for that: #154. The |
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.
+1 on moving forward as is and separating the other issue.
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.
Ok I'm good with log.logger
.
I would flesh out the description a little more, to clarify how this field is meant to be used. People are already getting confused by the amount of fields that can be used to describe the source of events. So I'd rather be a little more verbose.
Co-Authored-By: Mathieu Martin <[email protected]>
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.
I'm good with this, thanks Felix!
Did you want to wait for feedback from more people from the APM side?
Note that you'll have to pull this change, run |
I just wanted to confirm that Maybe wait another week to see if there are any objections and merge if there are? How do you normally handle that in the ECS repo? I just added a bunch of people as reviewers who might have a stake in this. |
I'm fine with waiting a little more. For merging, my heuristic is: I merge community PRs. Elasticians are free to merge, or ask me to merge, whichever they prefer. I'll be more proactive in taking over if there's backports needed (e.g. for small clarifications to fields that were in 1.0), as I don't want the burden of understanding the intricacies of the repo's branching & management to be pushed to all Elastic contributors. |
Let's get it in by end of day if no objects. We always have time up to the next release to change our opinion, but I prefer not to have pending PR's. |
As discussed in #281 (comment),
event.dataset
is not a good place to store the name of the logger. Let's create a more suitable field for that.