-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[logging] Format new platform json logging to ECS #71138
Conversation
Pinging @elastic/kibana-operations (Team:Operations) |
name: error.name, | ||
stack: error.stack, | ||
type: error.name, | ||
stack_trace: error.stack, | ||
}; | ||
} | ||
|
||
public format(record: LogRecord): string { | ||
return JSON.stringify({ | ||
'@timestamp': moment(record.timestamp).format('YYYY-MM-DDTHH:mm:ss.SSSZ'), |
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.
could you add a test that @timestamp
can be overridden? Should we @timestamp
value customization for pattern
layout as well?
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.
e095437 for the test. 👍 it makes sense to be consistent on the merge behavior. I'm going to circle back on this one in just a moment
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 took a closer look, the current behavior is open ended. I can replace @timestamp
. The pattern layout has client-side type conversions like formats and timezone for the default fields, and the meta object is serialized to json.
- merging the meta object into the record will throw a bunch of fields without formats at the logger. current behavior is json stringify, which will probably be strange for most ecs documents
- overrides before or after the converstion layer.
I'm leaning towards addressing 1 separately and for 2overriding before - there aren't any yet but we don't want to miss any escape or string filters.
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.
merging the meta object into the record will throw a bunch of fields without formats at the logger. current behavior is json stringify, which will probably be strange for most ecs documents
We stringify meta
fields in pattern
layout only, which is not subject to ECS format? However, I still don't see how users can reference data from the meta
object without using meta.
prefix.
I'm leaning towards addressing 1 separately and for 2overriding before - there aren't any yet but we don't want to miss any escape or string filters.
Are you going to create a separate issue?
error: JsonLayout.errorToSerializableObject(record.error), | ||
log: { | ||
level: record.level.id.toUpperCase(), | ||
logger: record.context, |
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.
How ES handles pattern layout for a given meta
object? Does it enforces ECS compatible property name usage in conversion patterns
? https://logging.apache.org/log4j/log4j-2.2/manual/layouts.html#PatternLayout
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.
For now the field usage is best effort and under migration. They'll eventually move to prefixing custom fields with es.
https://github.com/elastic/elasticsearch/blob/master/test/logger-usage/src/main/java/org/elasticsearch/test/loggerusage/ESLoggerUsageChecker.java does some static checking, but there's no strict compliance yet
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.
For now the field usage is best effort and under migration. They'll eventually move to prefixing custom fields with es.
What is our plan for this?
process: { | ||
pid: record.pid, | ||
}, | ||
...record.meta, |
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.
Is it possible to provide typings for ECS-compatible meta object?
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.
How detailed are you thinking? e.g. The whole spec, base objects, or defaults we're setting?
Might be worth digging into elastic/ecs#280
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.
Couple questions inline.
Another common property is the timings and request sizes. Those should be standardized as well.
pid: record.pid, | ||
}, | ||
}, | ||
record.meta |
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.
Similar to other comment, should we be placing record.meta
under labels
?
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.
meta might be named incorrectly - i tried to leave internal terminology as intact as possible, but it's closer to fields
or something. i could rename it
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.
Could you create an issue? I'm fine to rename it.
We do have timings with the For a little context on the organization part - a few questions would be
|
@elasticmachine merge upstream |
And then for the more specific logs - I'm thinking we'll want to provide the how and the structure, but mostly leave it up to the teams to decide. The surface area is quite large, we can certainly hook into some globals but there's other log levels. |
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.
LGTM - I still see a lot of work to do in regards to ensuring alignment with ECS but feel that is blocked on us migrating to the new logger.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
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.
For a little context on the organization part - a few questions would be
field order for pattern input
How ES handles the order? I suspect they ignore fields not listed in a pattern layout.
sanitizing output (validation)
Do you mean adding a filtering mechanism? #57547
input validation
yeah, I thought about this, but considering the number of logs, it could affect the runtime perf. You can add this suggestion to #58261
where the logger types live. does http live with the router or in the logger folder
type checking
It seems like ECS dictates the format. Even in ES, they use a dedicated builder living in the Logger (in their case in AuditLogger https://github.com/elastic/elasticsearch/blob/aa0148735e3384b23d504d44ed4f2e20f64f1bca/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java#L705)
pid: record.pid, | ||
}); | ||
return JSON.stringify( | ||
merge( |
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'd rather we didn't use lodash.merge
. It tripped us up several times with questionable functionality: for example, it merges array. BTW there is custom merge
implementation in the repo: https://github.com/restrry/kibana/blob/a9a2fae0fc0e56ed3c7f0985e38e6436e72e160f/src/core/utils/merge.ts#L27
return JSON.stringify( | ||
merge( | ||
{ | ||
'@timestamp': moment(record.timestamp).format('YYYY-MM-DDTHH:mm:ss.SSSZ'), |
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.
formatting should be applied to the user @timestamp
value as well. the same for error
, level
+ tests required
pid: record.pid, | ||
}, | ||
}, | ||
record.meta |
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.
Could you create an issue? I'm fine to rename it.
Spoke with restry on slack - I'm going to merge this now for FF. The logger isn't in use yet, and there's one bug and two renames I'll address during freeze (and more probably) I'm going to to recap and move^comments to the meta issue. I'll be following up with an implementation focusing on logging queries to elasticsearch. |
* [logging] Format new platform json logging to ECS * update integration tests * merge instead of assign * add @timestamp override test * add partial merge test against log object * add object level override test * fix type error Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: [Form lib] Memoize form hook object and fix hook array deps (elastic#71237) [uiActions] Support emitting nested triggers and actions (elastic#70602) add short sleep before clicking Remove on sample data (elastic#71104) Fixed the beta badge layout. (elastic#71835) Restores task for downloading Chromium builds (elastic#71749) [logging] Format new platform json logging to ECS (elastic#71138) add policy details and update SO limit requests (elastic#71789) Convert vis_type_vega to Typescript (elastic#68915) [ML] Fix UI Actions context menu positioning for the Anomaly Swim Lane (elastic#71839)
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
This updates the new platform JSON logger to be ECS compliant. Internal representation of log fields are left as is, so there's a few translations. In some cases this may make sense for objects we define, e.g. pid, but others (Error) are more questionable. I'm open to replacing some of these.
Objects passed in the meta field are assigned to provide extended fields beyond
message
.ecs reference
Closes #52226