Issue 6238 - RFE - add option to write audit log in JSON format #6239
Conversation
ldap/servers/slapd/auditlog.c
Outdated
| @@ -194,7 +199,7 @@ log_entry_attr(Slapi_Attr *entry_attr, char *attrname, lenstr *l) | |||
| { | |||
| Slapi_Value **vals = attr_get_present_values(entry_attr); | |||
| for(size_t i = 0; vals && vals[i]; i++) { | |||
| char log_val[256] = ""; | |||
| char log_val[256] = {0}; | |||
There was a problem hiding this comment.
Why changing that ?
Setting all 255 bytes to 0 instead of only setting the first one, seems like an useless overhead (especially since it is done for each values on each attribute.)
There was a problem hiding this comment.
Why changing that ? Setting all 255 bytes to 0 instead of only setting the first one, seems like an useless overhead (especially since it is done for each values on each attribute.)
I thought it was cleaner, but you have a good point. I'll change it
ldap/servers/slapd/auditlog.c
Outdated
| Slapi_Value **vals = attr_get_present_values(entry_attr); | ||
| for(size_t i = 0; vals && vals[i]; i++) { | ||
| json_object *attr_obj = json_object_new_object(); | ||
| char log_val[256] = {0}; |
There was a problem hiding this comment.
same remark, should probably better be char log_val[256] = "";
ldap/servers/slapd/auditlog.c
Outdated
| } else { | ||
| target_dn = "unknown"; | ||
| } | ||
| } else if (optype == SLAPI_OPERATION_DELETE) { |
There was a problem hiding this comment.
Duplicate case: if (optype == SLAPI_OPERATION_DELETE) {
should probably be a SLAPI_OPERATION_MODRDN)
There was a problem hiding this comment.
Duplicate case: if (optype == SLAPI_OPERATION_DELETE) { should probably be a SLAPI_OPERATION_MODRDN)
Nice catch!
|
|
||
| time_format = config_get_auditlog_time_format(); | ||
| (void)localtime_r(&curtime, &tms); | ||
| if (strftime(local_time, JBUFSIZE, time_format, &tms) == 0) { |
There was a problem hiding this comment.
Should we really convert the timestamp in the json version ?
The local time string format is better for the human, but json logs are intended for some post-processing tools
and IMHO the raw UTC time as integer is usually better for tools (Because each tool may then easily convert it the way it need it.)
For example the replication latency tool has to convert back the access log timestamp to integer because the different logs may be in different timezone)
And storing directly the integer is faster.
There was a problem hiding this comment.
Please check this discussion 389ds/389ds.github.io#14 (comment)
I asked for ISO 8601 Date string because it's unambiguous, human-readable, and well-supported.
There was a problem hiding this comment.
I agree with ISO 8601 but I do not think we should allow to change the format
and in any case it should be gmtime and not localtime.
IMHO allowing to change the format may not be a good idea.
Not only it add one more parameter, but there is a big difference between the two formats:
- The ascii output is intended to be directly readable by humans.
- The json output must be post-processed by a tools
And if the timestamp format may change, the tool developper will have to take care of the timestamp format
If it is only ISO 860, he has only to take care of ISO 8601 format.
So IMHO we should have a single ISO 8601 format and let the tool format the date
Sorry that I have not realized that point in April !
There was a problem hiding this comment.
@vashirov @progier389 The reason I made the time format configurable is because customers/community have been asking for this for years (for DS and IDM). I see no harm in having it, but if you guys think it's a bad idea then I will remove it (no problem).
There was a problem hiding this comment.
I agree with ISO 8601 but I do not think we should allow to change the format and in any case it should be gmtime and not localtime.
FYI, the "time" key, which is also the local time. This is how the current audit log records the time. So I kept that for legacy purposes and then introduced the "date" key for the custom time. Again, this is an open discussion but I just wanted clarify what the current PR is doing.
There was a problem hiding this comment.
You are right: I overlook the time key.
So forgot what i previously wrote: I do not really care about the way the date value is formatted ( local time and a configurable format is fine)
But for "time" you should use ISO 8601 as viktor mentionned.
(i.e: using strftime(isotime, (sizeof isotime), "%FT%TZ", gmtime(&curtime));
rather than format_localTime
There was a problem hiding this comment.
Oups! should rather use gmtime_r instead of gm_time in multi threaded environment ! 😉
Description: Add option to set the format between: default, json, or json-pretty json-pretty just writes the JSON format in a vertical structure verses one condensed line of text. You can also adjust the local time format using strftime formatting Relates: 389ds#6238 Reviewed by: ?
|
Ok all the changes have been made. I changed the names of the time keys from "date" -> "local_time", and "time" -> "gm_time". Not sure if those are the best names to use, but it's clearer than before. |
Description:
Add option to set the format between: default, json, or json-pretty
json-pretty just writes the JSON format in a vertical structure verses one condensed line of text.
You can also adjust the date format using strftime formatting
Relates: #6238