Re-factor network_event_log

chromeos/network/network_event_log.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROMEOS_NETWORK_NETWORK_EVENT_LOG_H_
 #define CHROMEOS_NETWORK_NETWORK_EVENT_LOG_H_
 
 #include <deque>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "chromeos/chromeos_export.h"
 
+namespace base {
+class Value;
+}
+
 namespace chromeos {
 
 // Namespace for functions for logging network events.
 namespace network_event_log {
 
 // Used to determine which order to output event entries in GetAsString.
 enum StringOrder {
   OLDEST_FIRST,
   NEWEST_FIRST
 };
 
+// Used to set the detail level for logging.
+enum LogLevel {
+  LOG_LEVEL_ERROR = 0,
+  LOG_LEVEL_EVENT = 1,
+  LOG_LEVEL_DEBUG = 2
+};
+
 // Maximum number of event log entries, exported for testing.
 CHROMEOS_EXPORT extern const size_t kMaxNetworkEventLogEntries;
 
+// Default log level.
+CHROMEOS_EXPORT extern const int kDefaultLogLevel;
+
 // Initializes / shuts down network event logging. Calling Initialize more than
 // once will reset the log.
 CHROMEOS_EXPORT void Initialize();
 CHROMEOS_EXPORT void Shutdown();
 
 // Returns true if network event logging has been initialized.
 CHROMEOS_EXPORT bool IsInitialized();
 
-// Adds an entry to the event log. A maximum number of events are recorded
-// after which new events replace old ones. Does nothing unless Initialize()
-// has been called.
-CHROMEOS_EXPORT void AddEntry(const std::string& module,
+// Adds an entry to the event log at level specified by |log_level|.
+// A maximum number of events are recorded after which new events replace
+// old ones. Does nothing unless Initialize() has been called.
+// NOTE: Generally use NET_LOG instead.
+CHROMEOS_EXPORT void AddEntry(const char* file,

[gauravsh, 2013/05/13 00:34:51]

When would one use this directly? If this is not meant to be used directly, lets
move this to an internal namespace to make that clearer.

[stevenjb, 2013/05/13 19:36:39]

The unit tests call it directly. Moved to 'internal'.

+                              int file_line,
+                              LogLevel log_level,
                               const std::string& event,
                               const std::string& description);
 
-// Outputs the log to a formatted string. |order| determines which order to
-// output the events. If |max_events| > 0, limits how many events are output.
-CHROMEOS_EXPORT std::string GetAsString(StringOrder order, size_t max_events);
+// Outputs the log to a formatted string.
+// |order| determines which order to output the events.
+// |format| is a string that determines which elelements to show. Elements
+// can be combined with or without separators, e.g. "time,desc".

[pneubeck (no reviews), 2013/05/13 13:16:25]

you're parsing the format string per entry. Since the rational behind this log
was high-frequent events: Please replace these strings by much more efficient
bit flags (in an enum) which can be bit-wise OR-ed and AND-ed.

(if there is a strong argument to continue with strings:
remove "without separators" and mention a specific separator. Otherwise this is
very fragile wrt. future changes.)

[stevenjb, 2013/05/13 19:36:39]

Made the optimization to only parse the string in GetAsString. Changed to an
explicit separator.

+// Note: order of the format strings does not affect the output.
+//  "time" - Include a timestamp.
+//  "file" - Include file and line number.
+//  "desc" - Include the description.
+//  "html" - Include html tags.
+// Only events with |log_level| >= |max_level| are included in the output.

[gauravsh, 2013/05/13 00:34:51]

This should be |log_level| <= |max_level|.

[stevenjb, 2013/05/13 19:36:39]

Done.

+// If |max_events| > 0, limits how many events are output.
+CHROMEOS_EXPORT std::string GetAsString(StringOrder order,
+                                        const std::string& format,
+                                        int max_level,
+                                        size_t max_events);
 
-// Macros to make logging format more consistent.
-#define NET_LOG(module, message)                                             \
-    ::chromeos::network_event_log::AddEntry(                                 \
-        module,                                                              \
-        std::string(__FILE__) + ":" + ::base::StringPrintf("%d",__LINE__) +  \
-        " (" + std::string(__func__) + ")",                                  \
-        message)
+// Utility function for displaying a DBus value as a string.
+CHROMEOS_EXPORT std::string DBusValueAsString(const base::Value& value);

[gauravsh, 2013/05/13 00:34:51]

NetworkEventLog is an odd place for this. I think this should be in its own
utility module with other helpers that work on *Value-s. (e.g. 
ManagedState::Get*Value , ConvertListValueToStringVector in network_state, etc.)

[pneubeck (no reviews), 2013/05/13 13:16:25]

There is already:
 operator<< in values.h
and
 JsonWriter

This function should at least delegate to one of these.
The method name is unnecessarily restrictive.

+1 to place this into a helper file that is as near to base/values.h as
possible, like base/values_util.h

[stevenjb, 2013/05/13 19:36:39]

Done.

 
-#define NET_LOG_WARNING(module, message)                                     \
-    NET_LOG(module, std::string("WARNING:") + message)
-#define NET_LOG_ERROR(module, message)                                       \
-    NET_LOG(module, std::string("ERROR:") + message)
+// Errors
+#define NET_LOG_ERROR(event, desc) NET_LOG_LEVEL(                       \
+    ::chromeos::network_event_log::LOG_LEVEL_ERROR, event, desc)
+
+// Important events, e.g. connection request
+#define NET_LOG_EVENT(event, desc) NET_LOG_LEVEL(                       \
+    ::chromeos::network_event_log::LOG_LEVEL_EVENT, event, desc)
+
+// Non-essencial debugging events

[gauravsh, 2013/05/13 00:34:51]

Non-essential

[stevenjb, 2013/05/13 19:36:39]

Done.

+#define NET_LOG_DEBUG(event, desc) NET_LOG_LEVEL(                       \
+    ::chromeos::network_event_log::LOG_LEVEL_DEBUG, event, desc)
+
+// Macro to include file and line number info in the event log.
+#define NET_LOG_LEVEL(log_level, event, description)            \
+  ::chromeos::network_event_log::AddEntry(                      \
+      __FILE__, __LINE__, log_level, event, description)

[pneubeck (no reviews), 2013/05/13 13:16:25]

should be FROM_HERE

[stevenjb, 2013/05/13 19:36:39]

FROM_HERE uses tracked_objects; I think that is more complexity than we want or
require for logging (see comment in ShillErrorCallbackFunction).

[pneubeck (no reviews), 2013/05/16 08:21:07]

What do you mean by "uses"? tracked_objects is the namespace.
Which complexity are you referring to?
E.g. the number of arguments is reduced => there is less to pass around => less
code complexity.


However, this is not essential. Feel free to leave it.

 
 }  // namespace network_event_log
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_NETWORK_EVENT_LOG_H_