chore(networkmonitor): address review comments 1

tools/networkmonitor/networkmonitor.nim

@@ -219,7 +219,12 @@ proc startRestApiServer(conf: NetworkMonitorConf,
 when isMainModule:
   # known issue: confutils.nim(775, 17) Error: can raise an unlisted exception: ref IOError
   {.pop.}
-  let conf = NetworkMonitorConf.load()
+  let confRes = NetworkMonitorConf.loadConfig()
+  if confRes.isErr():
+    error "could not load cli variables", err=confRes.error
+    quit(1)
+
+  let conf = confRes.get()
   info "cli flags", conf=conf
 
   if conf.logLevel != LogLevel.NONE:
@@ -230,9 +235,10 @@ when isMainModule:
 
   # start metrics server
   if conf.metricsServer:
-    startMetricsServer(
-      conf.metricsServerAddress,
-      Port(conf.metricsServerPort))
+    let res = startMetricsServer(conf.metricsServerAddress, Port(conf.metricsServerPort))
+    if res.isErr:
+      error "could not start metrics server", err=res.error
+      quit(1)
 
   # start rest server for custom metrics
   let res = startRestApiServer(conf, allPeersRef)

tools/networkmonitor/networkmonitor_config.nim

@@ -4,6 +4,7 @@ import
   chronicles/topics_registry,
   chronos,
   confutils,
+  stew/results,
   stew/shims/net
 
 type
@@ -72,7 +73,14 @@ proc parseCmdArg*(T: type chronos.Duration, p: string): T =
   try:
     result = chronos.seconds(parseInt(p))
   except CatchableError as e:
-    raise newException(ConfigurationError, "Invalid timeout value")
+    raise newException(ConfigurationError, "Invalid duration value")
 
 proc completeCmdArg*(T: type chronos.Duration, val: string): seq[string] =
-  return @[]
+  return @[]
+
+proc loadConfig*(T: type NetworkMonitorConf): Result[T, string] =
+  try:
+    let conf = NetworkMonitorConf.load()
+    ok(conf)
+  except CatchableError:
+    err(getCurrentExceptionMsg())

tools/networkmonitor/networkmonitor_metrics.nim

@@ -8,19 +8,17 @@ import
   chronicles,
   chronicles/topics_registry,
   chronos,
+  json_serialization,
   metrics,
   metrics/chronos_httpserver,
   presto/route,
   presto/server,
+  stew/results,
   stew/shims/net
 
 logScope:
   topics = "networkmonitor_metrics"
 
-# Metric ideas:
-# histogram with latency
-# number of peers hosted behind each ip
-
 # On top of our custom metrics, the following are reused from nim-eth
 #routing_table_nodes{state=""}
 #routing_table_nodes{state="seen"}
@@ -55,21 +53,21 @@ type
     supportedProtocols*: seq[string]
     userAgent*: string
 
-  CustomPeersTable* = Table[string, CustomPeerInfo]
-  CustomPeersTableRef* = ref CustomPeersTable
+  CustomPeersTableRef* = TableRef[string, CustomPeerInfo]
 
 # GET /allpeersinfo
 proc installHandler*(router: var RestRouter, allPeers: CustomPeersTableRef) =
   router.api(MethodGet, "/allpeersinfo") do () -> RestApiResponse:
-    let values = toSeq(allPeers.keys()).mapIt(allPeers[it])
-    return RestApiResponse.response($(%values), contentType="application/json")
+    let values = toSeq(allPeers.values())
+    return RestApiResponse.response(values.toJson(), contentType="application/json")
 
-proc startMetricsServer*(serverIp: ValidIpAddress, serverPort: Port) =
+proc startMetricsServer*(serverIp: ValidIpAddress, serverPort: Port): Result[void, string] =
     info "Starting metrics HTTP server", serverIp, serverPort
 
     try:
       startMetricsHttpServer($serverIp, serverPort)
     except Exception as e:
-      raiseAssert("Exception while starting metrics HTTP server: " & e.msg)
+      error("Failed to start metrics HTTP server", serverIp=serverIp, serverPort=serverPort, msg=e.msg)
 
-    info "Metrics HTTP server started", serverIp, serverPort
+    info "Metrics HTTP server started", serverIp, serverPort
+    ok()

tools/networkmonitor/networkmonitor_utils.nim

@@ -26,6 +26,7 @@ proc flatten*[T](a: seq[seq[T]]): seq[T] =
 
 # using an external api retrieves some data associated with the ip
 # TODO: use a cache
+# TODO: use nim-presto's HTTP asynchronous client
 proc ipToLocation*(ip: string,
                    client: Httpclient):
                    Future[Result[NodeLocation, string]] {.async.} =
@@ -48,5 +49,5 @@ proc ipToLocation*(ip: string,
       isp:     jsonContent["isp"].getStr()
     ))
   except:
-    error "failed to get location for IP", ip=ip, excep=getCurrentException().msg
-    return err("could not get location for IP: " & ip)
+    error "failed to get the location for IP", ip=ip, error=getCurrentExceptionMsg()
+    return err("failed to get the location for IP '" & ip & "':" & getCurrentExceptionMsg())