[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-trafficcontrol/pull/167


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r96073860
  
--- Diff: traffic_stats/build/traffic_stats.spec ---
@@ -75,8 +75,8 @@ 
godir=src/github.com/apache/incubator-trafficcontrol/traffic_stats/influxdb_tool
 ( mkdir -p "$godir" && \
   cd "$godir" && \
   cp -r "$TC_DIR"/traffic_stats/influxdb_tools/* . && \
-  go build sync_ts_databases.go
-  go build create_ts_databases.go
+  go build sync/sync_ts_databases.go
+  go build create/create_ts_databases.go
--- End diff --

@dneuman64 Removed and pushed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[dneuman64]

Github user dneuman64 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r96072989
  
--- Diff: traffic_stats/build/traffic_stats.spec ---
@@ -75,8 +75,8 @@ 
godir=src/github.com/apache/incubator-trafficcontrol/traffic_stats/influxdb_tool
 ( mkdir -p "$godir" && \
   cd "$godir" && \
   cp -r "$TC_DIR"/traffic_stats/influxdb_tools/* . && \
-  go build sync_ts_databases.go
-  go build create_ts_databases.go
+  go build sync/sync_ts_databases.go
+  go build create/create_ts_databases.go
--- End diff --

It won't let me comment on line 69, but it needs to be removed since we are 
vendoring now.
`go_get_version "$oldpwd"/src/github.com/influxdata/influxdb v1.0.1 && \`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95610649
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

@dneuman64 @rob05c I think that's valid. Thanks for the replies, I'll make 
the switch.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[dneuman64]

Github user dneuman64 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95588778
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

IMO we should just use the standard testing package for this code.  This 
code is really more a set of loosely related scripts than a "program" which is 
why there wasn't any unit tests to begin with. Additionally, the logic in this 
code isn't changing all that often so encouraging people to write their own 
tests is not a concern to me.  If we want to explore the idea of using a test 
framework for our unit tests I think the Traffic Ops client code or goTM 
(Traffic_monitor/experimental) are better candidates.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95499191
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

Sounds good. I'm working on moving some of the common influxdb code out, 
and creating some "autpopulating" config objects (with flag.Parse(), anyways).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95498458
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

I'm deferring to @rob05c and @dewrich on the testing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95489426
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

@rob05c @dewrich @PSUdaemon Any thoughts on the above? I can undo 
`goconvey`, move it to `testify`, or just go with the more verbose testing 
option, but it'd be nice to only do the work once.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[dewrich]

Github user dewrich commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95182473
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

I've used GoConvey to monitor test coverage, but I'm not sure about the 
value add of tying the code to the framework.  I've typically just wrapped the 
Go testing framework with convenience methods when necessary.  We haven't had a 
larger discussion about these types of frameworks to weigh the pros/cons.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95100817
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

I think @dewrich and @rob05c should weigh in here too. If using a framework 
like this means we get more people to contribute tests, I think that's 
something we should take into consideration too.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95100599
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
+   cli.StringFlag{
+   Name:  "url",
+   Usage: "The influxdb url and port",
+   Value: "http://localhost:8086;,
+   },
+   cli.IntFlag{
+   Name:  "replication",
+   Usage: "The number of nodes in the cluster",
+   Value: 3,
+   },
+   cli.StringFlag{
+   Name:  "user",
+   Usage: "The influxdb username used to create DBs",
+   Value: "",
+   },
+   cli.StringFlag{
+   Name:  "password",
+   Usage: "The influxdb password used to create DBs",
+   Value: "",
+   },
+   }
+}
+
+func create(c *cli.Context) error {
+   influxURL := c.String("url")
+   replication := c.Int("replication")
+   user := c.String("user")
+   password := c.String("password")
 
-   influxURL := flag.String("url", "http://localhost:8086;, "The influxdb 
url and port")
-   replication := flag.String("replication", "3", "The number of nodes in 
the cluster")
-   user := flag.String("user", "", "The influxdb username used to create 
DBs")
-   password := flag.String("password", "", "The influxdb password used to 
create DBs")
-   flag.Parse()
-   fmt.Printf("creating datbases for influxUrl: %v with a replication of 
%v using user %s\n", *influxURL, *replication, *user)
+   fmt.Printf("creating datbases for influxUrl: %s with a replication of 
%d using user %s\n", influxURL, replication, user)
client, err := influx.NewHTTPClient(influx.HTTPConfig{
-   Addr: *influxURL,
-   Username: *user,
-   Password: *password,
+   Addr: influxURL,
+   Username: user,
+   Password: password,
})
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
_, _, err = client.Ping(10)
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
 
createCacheStats(client, replication)
createDailyStats(client, replication)
createDeliveryServiceStats(client, replication)
+   return nil
 }
 
-func queryDB(client influx.Client, cmd string) (res []influx.Result, err 
error) {
+// queryDB takes a variadic argument for the target database so as to make
+// passing the variable optional, however, if passed, only the first db 
passed
+// in will be used
+func queryDB(client influx.Client, cmd string, dbs ...string) (res 
[]influx.Result, err error) {
+   db := ""
+   if len(dbs) > 0 {
+   db = dbs[0]
--- End diff --

Both. I think `traffic_stats.go` also uses a similar function.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95100626
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

I'd say clarity of tests (rather than saying something like 
```go
if val != expected {
   t.Fail()
}
```
you can say something like 
```go
So(val, ShouldEqual, expectedVal)
```
), with several keywords (`ShouldContain`, `ShouldBeNil`, `ShouldNotBeNil`, 
etc.), as well as the availability of the context for testing, so that 
anonymous functions (say in a test httpHandler at test time) can make 
assertions as well. 

That said, the built-in test framework can definitely be used. My thought 
with the addition was mostly that it makes tests easier to reason about and 
maybe more approachable? If the aim is to minimize external dependencies, which 
I understand, I'd rewrite these with the standard testing library.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95100395
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
+   cli.StringFlag{
+   Name:  "url",
+   Usage: "The influxdb url and port",
+   Value: "http://localhost:8086;,
+   },
+   cli.IntFlag{
+   Name:  "replication",
+   Usage: "The number of nodes in the cluster",
+   Value: 3,
+   },
+   cli.StringFlag{
+   Name:  "user",
+   Usage: "The influxdb username used to create DBs",
+   Value: "",
+   },
+   cli.StringFlag{
+   Name:  "password",
+   Usage: "The influxdb password used to create DBs",
+   Value: "",
+   },
+   }
+}
+
+func create(c *cli.Context) error {
+   influxURL := c.String("url")
+   replication := c.Int("replication")
+   user := c.String("user")
+   password := c.String("password")
 
-   influxURL := flag.String("url", "http://localhost:8086;, "The influxdb 
url and port")
-   replication := flag.String("replication", "3", "The number of nodes in 
the cluster")
-   user := flag.String("user", "", "The influxdb username used to create 
DBs")
-   password := flag.String("password", "", "The influxdb password used to 
create DBs")
-   flag.Parse()
-   fmt.Printf("creating datbases for influxUrl: %v with a replication of 
%v using user %s\n", *influxURL, *replication, *user)
+   fmt.Printf("creating datbases for influxUrl: %s with a replication of 
%d using user %s\n", influxURL, replication, user)
client, err := influx.NewHTTPClient(influx.HTTPConfig{
-   Addr: *influxURL,
-   Username: *user,
-   Password: *password,
+   Addr: influxURL,
+   Username: user,
+   Password: password,
})
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
_, _, err = client.Ping(10)
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
 
createCacheStats(client, replication)
createDailyStats(client, replication)
createDeliveryServiceStats(client, replication)
+   return nil
 }
 
-func queryDB(client influx.Client, cmd string) (res []influx.Result, err 
error) {
+// queryDB takes a variadic argument for the target database so as to make
+// passing the variable optional, however, if passed, only the first db 
passed
+// in will be used
+func queryDB(client influx.Client, cmd string, dbs ...string) (res 
[]influx.Result, err error) {
+   db := ""
+   if len(dbs) > 0 {
+   db = dbs[0]
--- End diff --

You'd prefer the fix in terms of sharing the code, or just the change from 
variadic to slice input? I'll be happy to make either (but moreso the former)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95099075
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
--- End diff --

We aren't shy about taking contributions, so breaking it up into smaller 
more manageable chunks is preferred. That way we can more easily get code 
committed rather than trying to manage different facets of a large PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95098650
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
+   cli.StringFlag{
+   Name:  "url",
+   Usage: "The influxdb url and port",
+   Value: "http://localhost:8086;,
+   },
+   cli.IntFlag{
+   Name:  "replication",
+   Usage: "The number of nodes in the cluster",
+   Value: 3,
+   },
+   cli.StringFlag{
+   Name:  "user",
+   Usage: "The influxdb username used to create DBs",
+   Value: "",
+   },
+   cli.StringFlag{
+   Name:  "password",
+   Usage: "The influxdb password used to create DBs",
+   Value: "",
+   },
+   }
+}
+
+func create(c *cli.Context) error {
+   influxURL := c.String("url")
+   replication := c.Int("replication")
+   user := c.String("user")
+   password := c.String("password")
 
-   influxURL := flag.String("url", "http://localhost:8086;, "The influxdb 
url and port")
-   replication := flag.String("replication", "3", "The number of nodes in 
the cluster")
-   user := flag.String("user", "", "The influxdb username used to create 
DBs")
-   password := flag.String("password", "", "The influxdb password used to 
create DBs")
-   flag.Parse()
-   fmt.Printf("creating datbases for influxUrl: %v with a replication of 
%v using user %s\n", *influxURL, *replication, *user)
+   fmt.Printf("creating datbases for influxUrl: %s with a replication of 
%d using user %s\n", influxURL, replication, user)
client, err := influx.NewHTTPClient(influx.HTTPConfig{
-   Addr: *influxURL,
-   Username: *user,
-   Password: *password,
+   Addr: influxURL,
+   Username: user,
+   Password: password,
})
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
_, _, err = client.Ping(10)
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
 
createCacheStats(client, replication)
createDailyStats(client, replication)
createDeliveryServiceStats(client, replication)
+   return nil
 }
 
-func queryDB(client influx.Client, cmd string) (res []influx.Result, err 
error) {
+// queryDB takes a variadic argument for the target database so as to make
+// passing the variable optional, however, if passed, only the first db 
passed
+// in will be used
+func queryDB(client influx.Client, cmd string, dbs ...string) (res 
[]influx.Result, err error) {
+   db := ""
+   if len(dbs) > 0 {
+   db = dbs[0]
--- End diff --

I'm all for reuse of code. There aren't any API contracts we need to adhere 
to. I'd prefer to just fix these outright.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95098546
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

We could definitely use more testing. What does this get us over the built 
in testing framework?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95096773
  
--- Diff: traffic_stats/influxdb_tools/sync/sync_test.go ---
@@ -0,0 +1,230 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+package main
+
+import (
+   "encoding/json"
+   "fmt"
+   "testing"
+
+   influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/influxdata/influxdb/models"
+   . "github.com/smartystreets/goconvey/convey"
--- End diff --

The new dependency here is 
[goconvey](https://github.com/smartystreets/goconvey), which in my experience 
makes unit tests more accessible. Had there been an existing testing framework 
in use, I would have used that, but in lieu of one, I just used one that I 
consider to facilitate the creation of unit tests, since ease of use can 
sometimes drive adoption (so that the package gets better coverage).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95096692
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
+   cli.StringFlag{
+   Name:  "url",
+   Usage: "The influxdb url and port",
+   Value: "http://localhost:8086;,
+   },
+   cli.IntFlag{
+   Name:  "replication",
+   Usage: "The number of nodes in the cluster",
+   Value: 3,
+   },
+   cli.StringFlag{
+   Name:  "user",
+   Usage: "The influxdb username used to create DBs",
+   Value: "",
+   },
+   cli.StringFlag{
+   Name:  "password",
+   Usage: "The influxdb password used to create DBs",
+   Value: "",
+   },
+   }
+}
+
+func create(c *cli.Context) error {
+   influxURL := c.String("url")
+   replication := c.Int("replication")
+   user := c.String("user")
+   password := c.String("password")
 
-   influxURL := flag.String("url", "http://localhost:8086;, "The influxdb 
url and port")
-   replication := flag.String("replication", "3", "The number of nodes in 
the cluster")
-   user := flag.String("user", "", "The influxdb username used to create 
DBs")
-   password := flag.String("password", "", "The influxdb password used to 
create DBs")
-   flag.Parse()
-   fmt.Printf("creating datbases for influxUrl: %v with a replication of 
%v using user %s\n", *influxURL, *replication, *user)
+   fmt.Printf("creating datbases for influxUrl: %s with a replication of 
%d using user %s\n", influxURL, replication, user)
client, err := influx.NewHTTPClient(influx.HTTPConfig{
-   Addr: *influxURL,
-   Username: *user,
-   Password: *password,
+   Addr: influxURL,
+   Username: user,
+   Password: password,
})
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
_, _, err = client.Ping(10)
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
 
createCacheStats(client, replication)
createDailyStats(client, replication)
createDeliveryServiceStats(client, replication)
+   return nil
 }
 
-func queryDB(client influx.Client, cmd string) (res []influx.Result, err 
error) {
+// queryDB takes a variadic argument for the target database so as to make
+// passing the variable optional, however, if passed, only the first db 
passed
+// in will be used
+func queryDB(client influx.Client, cmd string, dbs ...string) (res 
[]influx.Result, err error) {
+   db := ""
+   if len(dbs) > 0 {
+   db = dbs[0]
--- End diff --

This is related to the changes I was hoping to add on top of this, in terms 
of trying to generalize the code currently in use to allow for code reuse 
(previously there were two separate functions, one allowing for `db` and one 
not, with the latter's body being identical except for instantiating the 
`influx.Query` passed an empty string instead. 

The change to a slice may well be better (but best would be the 
generalization of multiple queries then, and the reuse of common code). 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

Github user sbogacz commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95096542
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
--- End diff --

That's definitely a premature optimization which I'd be happy to swap for a 
named var. Since there were no tests to start with, I had to A/B test most of 
the changes I was making. My "long" term goal was to abstract out something 
like a 
```go
type DBConfig struct {
url string
replication int
user string
password string
} 

func flagName(prefix, name string) string {
   if prefix == "" {
  return name
   }
   return fmt.Sprintf("%s-$s", prefix, name)
}

func envName(prefix, name string) string {
   if prefix == "" {
  return strings.ToUpper(name)
   }
   return strings.ToUpper(fmt.Sprintf("%s_$s", prefix, name))
}

func (c *DBConfig) Flags(prefix string) []cli.Flag {
return []cli.Flag{
cli.StringFlag{
Name:  flagName(prefix, "url"),
EnvVar: envName(prefix, "url"),
Usage: "The influxdb url and port",
Value: "http://localhost:8086;,
Destination: ,
},
...
}
}
```

Which would allow the start of some abstraction between the two commands, 
however, I realized part way that my changes were already larger than 
recommended, and that the course I was taking at the time (one executable with 
two commands) would actually break the build script as it stands.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95070933
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
--- End diff --

Why new deps?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95070929
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
--- End diff --

Why clean up other constants, but put this into a function just to return a 
constant?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/167#discussion_r95070920
  
--- Diff: traffic_stats/influxdb_tools/create/create_ts_databases.go ---
@@ -20,45 +20,81 @@ under the License.
 package main
 
 import (
-   "flag"
"fmt"
-   "os"
 
influx "github.com/influxdata/influxdb/client/v2"
+   "github.com/pkg/errors"
+   "github.com/urfave/cli"
 )
 
-func main() {
+const (
+   cache   = "cache_stats"
+   deliveryService = "deliveryservice_stats"
+   daily   = "daily_stats"
+)
+
+func createFlags() []cli.Flag {
+   return []cli.Flag{
+   cli.StringFlag{
+   Name:  "url",
+   Usage: "The influxdb url and port",
+   Value: "http://localhost:8086;,
+   },
+   cli.IntFlag{
+   Name:  "replication",
+   Usage: "The number of nodes in the cluster",
+   Value: 3,
+   },
+   cli.StringFlag{
+   Name:  "user",
+   Usage: "The influxdb username used to create DBs",
+   Value: "",
+   },
+   cli.StringFlag{
+   Name:  "password",
+   Usage: "The influxdb password used to create DBs",
+   Value: "",
+   },
+   }
+}
+
+func create(c *cli.Context) error {
+   influxURL := c.String("url")
+   replication := c.Int("replication")
+   user := c.String("user")
+   password := c.String("password")
 
-   influxURL := flag.String("url", "http://localhost:8086;, "The influxdb 
url and port")
-   replication := flag.String("replication", "3", "The number of nodes in 
the cluster")
-   user := flag.String("user", "", "The influxdb username used to create 
DBs")
-   password := flag.String("password", "", "The influxdb password used to 
create DBs")
-   flag.Parse()
-   fmt.Printf("creating datbases for influxUrl: %v with a replication of 
%v using user %s\n", *influxURL, *replication, *user)
+   fmt.Printf("creating datbases for influxUrl: %s with a replication of 
%d using user %s\n", influxURL, replication, user)
client, err := influx.NewHTTPClient(influx.HTTPConfig{
-   Addr: *influxURL,
-   Username: *user,
-   Password: *password,
+   Addr: influxURL,
+   Username: user,
+   Password: password,
})
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
_, _, err = client.Ping(10)
if err != nil {
-   fmt.Printf("Error creating influx client: %v\n", err)
-   os.Exit(1)
+   return errors.Wrap(err, "Error creating influx client")
}
 
createCacheStats(client, replication)
createDailyStats(client, replication)
createDeliveryServiceStats(client, replication)
+   return nil
 }
 
-func queryDB(client influx.Client, cmd string) (res []influx.Result, err 
error) {
+// queryDB takes a variadic argument for the target database so as to make
+// passing the variable optional, however, if passed, only the first db 
passed
+// in will be used
+func queryDB(client influx.Client, cmd string, dbs ...string) (res 
[]influx.Result, err error) {
+   db := ""
+   if len(dbs) > 0 {
+   db = dbs[0]
--- End diff --

Why take a variadic argument if you will only ever use the first one? This 
seems like you want to have function overloading in a language that doesn't 
support it. It seems more idiomatic to pass a slice that can be empty.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #167: Refactor and cleanup the influxd...

[sbogacz]

GitHub user sbogacz opened a pull request:

https://github.com/apache/incubator-trafficcontrol/pull/167

Refactor and cleanup the influxdb_tools in traffic_stats

1) Cleanup lint warnings in the traffic_stats/influxdb_tools go files 
(mostly due to repeated constant strings), and fix some idiomatic issues, like 
using both the  `+` operator and `fmt.Sprintf()` to concatenate strings on the 
same line.

2) Use the [errors package](https://github.com/pkg/errors) to report and 
add on to errors, rather than maintaining a global `errorMessage`.

3) Move the sync and create tools into separate folders so that the code 
can be tested (otherwise the compiler complains that main() and queryDB() are 
duplicated)

4) Use the [cli package](https://github.com/urfave/cli) to make the 
somewhat more user friendly. 

5) Test the `get*Stats` methods to ensure they behave accordingly, fix 
panic issues where a nil or empty slice gets passed in to those functions and 
the function doesn't check, and actually iterate over entire input slice, 
rather than only first result element

6) Add test file for traffic_stats.go, mostly to provide a starting point.

There's a few things going on in this PR, I got carried away and will have 
to open another subsequent PR for adding tests (and some refactoring if it's 
needed to make the code testable).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/sbogacz/incubator-trafficcontrol master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-trafficcontrol/pull/167.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #167


commit 38135b93e28ef383715ec45284eb56fdc8662a81
Author: sbogacz 
Date:   2017-01-07T00:06:43Z

Clean up linter warnings, and use only one method of string concatenation 
per line

commit 739edba62061905e5b19cdd518fb3278510c8689
Author: sbogacz 
Date:   2017-01-07T02:10:50Z

Refactor influxdb_tools to be a go buildable executable (e.g. go build -o 
influxtools) with create and sync commands. By reducing to a single main 
function in the package, we also allow for package level tests to get the 
coverage up

commit 5f804224399ab917047eb46e8ff3bcedf30f8d67
Author: sbogacz 
Date:   2017-01-07T02:13:42Z

Recycle the queryDB function from the create code in the sync code

commit e360e3d602ba46e8212c2214a6c592848e11b820
Author: sbogacz 
Date:   2017-01-07T02:25:38Z

Remove unnecessary global errorMessage variable, use fmt.Printf() instead

commit 4fcbb0bd0d7f80107d96e0eb524f66e5a02c
Author: sbogacz 
Date:   2017-01-07T06:35:59Z

Refactor influxdb_tools to have separate folders for the different 
executables (so as not to break the current build script, in comparison to 
separate commands on a single cli.App

commit 798252c9711b91a233cf87468cbaa6cdc6b33132
Author: sbogacz 
Date:   2017-01-07T06:39:07Z

Fix build script to reflect new locations of influxdb_tools go files

commit 185273da310f51a1bca065b18590d5382bb727b0
Author: sbogacz 
Date:   2017-01-07T07:16:34Z

When using iota in a cons() block, it need only be called once




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---