Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.origin fields #563
Add log.origin fields #563
Changes from 3 commits
89c1ac8
3d0c917
60b1ee1
ea47d83
75c32b4
f2a19d3
1ec53dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Let's reuse the
file
object here: https://github.com/elastic/ecs/blob/master/schemas/file.yml#L15So this becomes
log.origin.file.path
I guess?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.
Good call! It would be
log.origin.file.name
though. Should we then also dolog.origin.file.line
instead oflog.origin.line
?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.
That would make sense I guess. I wonder if we "must" extend
file
for this or we just use it here? I'm good with just adding it here but I'm sure at @webmat will have an opinion on this.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 fine with directly adding 2-3 specific fields for now 👍
We're going for something like this for
package.*
as wellThere 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.
Note: Using the
file.*
object here would require a change to ECS, since it is not currently defined as a reusable (nestable) object. I'm not saying this is wrong, as the idea has come up before to capture events like file-copy events, but we should consider this as we work through this PR, addressing where elsefile.*
could be nested. For now, I would suggest we don't re-use thefile.*
object here.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.
It's not using the full
file
object here but re-uses the same structure. Coming up with slightly different names just not to re-use thefile
object structure doesn't feel quite right. But I think I don't quite understand what you have in mind here. Which names would you suggest?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.
Out of curiosity: If it's a method, would we expect it to also indicate what class it belongs to? Something like
Foo.bar
?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.
No, it would just be the
bar
in this case. We could, however, add an optionalorigin.class
for languages which support that concept.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.
Got it, SGTM. Lets only add it if we already have a use case for it. But
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.
We actually do have a use case for logging. For java logging libraries, we usually get both the file name and the class name of the origin. See also https://docs.oracle.com/javase/7/docs/api/java/lang/StackTraceElement.html#getClassName().
I didn't add it at first because not all languages have the concept of a class. But when it's optional, why not?
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.
Yeah, I could name quite a few that have a similar concept ;-) In the case of Golang I would also use
.class
even though that is not 100% correct. @axw Hope you agree :-)+1 on adding class directly.
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 fine with class as a name, but I would have made it a qualified name like
Foo.bar
as you described. @felixbarny what was the reason behind excluding the class name? Is it useful to search on them independently?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 don't see a reason why the class name should be included
log.origin.function
, am I missing something?We can definitely have an additional field for the class (
log.origin.class
) which contains the fully qualified class name likeorg.elasticsearch.bootstrap.Bootstrap
. But we currently don't have that field in the APM schema where we have module instead, which would translate to a Java package.I'd like to defer these potentially controversial discussions and get the lowest common denominator in which, I think, is
log.origin.file.name
,log.origin.file.line
andlog.origin.function
.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.
In the description it mentions, it can be a function or method. So if it is a method, would it be wrong to put here
Bootstrap.foo
? What I'm getting at, it would be nice if the shortcut to this discussion is that both options are fine. Note: I left out the package name on purpose, was not even thinking of it before @felixbarny brought up the example.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.
That would seem a bit weird to me as the method name is foo, not Bootstrap.foo. https://docs.oracle.com/javase/7/docs/api/java/lang/StackTraceElement.html#getMethodName() would also just return the method name.
What would be the benefit of concatenating the simple class name and the method name?
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.
@felixbarny I'm not too fussed if this is deferred. I don't like to sound like a broken record, but the use case I have in mind is once again profiling, and I was hoping we could find a common definition of fields.
I'd say the benefit of having them combined is having a simple display name, so that UIs don't need to know how to recombine the class and method name. e.g. it's "Class::Method" in C++.