You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/08/27 19:39:09 UTC
[GitHub] [trafficcontrol] ezelkow1 opened a new pull request #4993: Adds support for CSV parsing in astats
ezelkow1 opened a new pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
Adds CSV parsing support to traffic monitor for the astats plugin.
The astats parser will check the incoming content-type header to determine if it should parse the data as json or csv. Added a cfg option of http_polling_format that defaults to "text/json", this is what is sent on http requests to caches. Changing it to "text/csv" will enable the csv output if the cache's astats plugin as has support for CSV, otherwise astats will respond with "text/json" or "text/javascript" depending on the version.
- [x] This PR fixes #REPLACE_ME OR is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Documentation
- Traffic Monitor
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
Set the http_polling_format configuration option in TM to "text/csv" to send that in the accept header to caches. Using a cache that has a recent version of astats with CSV support then verify that the cache and its stats are reported correctly in TM.
Tests exist in the traffic_monitor/cache directory
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
I have also included benchmarks for comparison to json usage:
BenchmarkAstatsJson-8 5931 203527 ns/op 134475 B/op 2028 allocs/op
BenchmarkAstatsCSV-8 8680 147683 ns/op 119839 B/op 812 allocs/op
The CSV is ~40-50% faster depending on run while also greatly decreasing the amount of allocations per operation which should hopefully help with garbage collection pressures.
<!--
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.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#issuecomment-686070547
> > * [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
>
> Since this is a new feature, would you please add a changelog entry?
Opened https://github.com/apache/trafficcontrol/pull/5009
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480212612
##########
File path: traffic_monitor/cache/astats.csv
##########
@@ -0,0 +1,527 @@
+proxy.process.http.completed_requests,26220072200
Review comment:
Right but its also not indicative of what the astats plugin would normally return, besides csv was normally excluded to begin with
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480381731
##########
File path: traffic_monitor/cache/astats_test.go
##########
@@ -60,14 +57,11 @@ func TestAstatsCSV(t *testing.T) {
pl := &poller.HTTPPollCtx{HTTPHeader: http.Header{}}
ctx := interface{}(pl)
ctx.(*poller.HTTPPollCtx).HTTPHeader.Set("Content-Type", "text/csv")
- _, thismap, err := astatsParse("testCache", file, ctx)
+ _, _, err = astatsParse("testCache", file, ctx)
Review comment:
We still want to verify the correct size of the map, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480211422
##########
File path: traffic_monitor/cache/stats_types.go
##########
@@ -105,7 +105,7 @@ type StatsDecoder struct {
// whatever miscellaneous data was in the payload but not represented by
// the properties of a Statistics object, so that it can be used in later
// calculations if necessary.
-type StatisticsParser func(string, io.Reader) (Statistics, map[string]interface{}, error)
+type StatisticsParser func(string, io.Reader, interface{}) (Statistics, map[string]interface{}, error)
Review comment:
I left it as interface{} since this is the generic definition of a parser, so it gets used by any potential stats parser plugins. So specifying it as httppoll ties peoples hands as to what a plugin would be able to accept. So I think its best to leave it as generic as possible
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480198408
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
+ }
+
+ statMap := atsData.Ats
+
+ // Handle system specific values and remove them from the map for precomputing to not have issues
+ if stats.Loadavg, err = LoadavgFromRawLine(statMap["proc.loadavg"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Error parsing loadavg for cache '%s': %v", cacheName, err)
Review comment:
To add some context as to why: it's so they compose well. For example, if the parent function did
`return errors.New("doing foo: " + err.Error())`, it ends up as a pretty sentence, "doing foo: parsing loadavg...", and the final `log.Errorln` can/should start with a capital (and could add a period at the end, though people don't usually bother with that).
As a nitpick to the nitpick, the word "Error" here is superfluous for the same reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480198408
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
+ }
+
+ statMap := atsData.Ats
+
+ // Handle system specific values and remove them from the map for precomputing to not have issues
+ if stats.Loadavg, err = LoadavgFromRawLine(statMap["proc.loadavg"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Error parsing loadavg for cache '%s': %v", cacheName, err)
Review comment:
To add some context as to why: it's so they compose well. For example, if the parent function did
`return errors.New("doing foo: " + err.Error())`, it ends up as a pretty sentence, "doing foo: parsing loadavg...", and the final `log.Errorln` (or however it's returned to the user) can/should start with a capital (and could add a period at the end, though people don't usually bother with that).
As a nitpick to the nitpick, the word "Error" here is superfluous for the same reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480397324
##########
File path: traffic_monitor/cache/astats_test.go
##########
@@ -60,14 +57,11 @@ func TestAstatsCSV(t *testing.T) {
pl := &poller.HTTPPollCtx{HTTPHeader: http.Header{}}
ctx := interface{}(pl)
ctx.(*poller.HTTPPollCtx).HTTPHeader.Set("Content-Type", "text/csv")
- _, thismap, err := astatsParse("testCache", file, ctx)
+ _, _, err = astatsParse("testCache", file, ctx)
Review comment:
its more important that it just doesnt crash. There are already differences between both the json and csv outputs because the CSV one will strip out extraneous fields. Just verifying the size doesnt mean much since you could throw some random data into the files and still have it be allocated in to a proper map
This is also all the original test was doing as well, only checking that it didnt crash, beyond printing a value that wasnt checked or useful for anything, however it was only checking the jsoniter unmarshal capability and not checking that the TM functions themselves didnt cause issues
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480168290
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
Review comment:
`pollCTX` is unused
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
+ }
+
+ statMap := atsData.Ats
+
+ // Handle system specific values and remove them from the map for precomputing to not have issues
+ if stats.Loadavg, err = LoadavgFromRawLine(statMap["proc.loadavg"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Error parsing loadavg for cache '%s': %v", cacheName, err)
+ } else {
+ delete(statMap, "proc.loadavg")
+ }
+
+ if err := stats.AddInterfaceFromRawLine(statMap["proc.net.dev"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
+ } else {
+ delete(statMap, "proc.net.dev")
+ }
+
+ if inf, ok := stats.Interfaces[statMap["inf.name"].(string)]; !ok {
+ return stats, nil, errors.New("/proc/net/dev line didn't match reported interface line")
+ } else {
+ inf.Speed = int64(statMap["inf.speed"].(float64)) //strconv.ParseInt(statMap["inf.speed"].(string), 10, 64)
+ stats.Interfaces[statMap["inf.name"].(string)] = inf
+ delete(statMap, "inf.speed")
+ delete(statMap, "inf.name")
+
+ }
+
+ // Clean up other non-stats entries
+ delete(statMap, "astatsLoad")
+ delete(statMap, "lastReloadRequest")
+ delete(statMap, "version")
+ delete(statMap, "something")
+ delete(statMap, "lastReload")
+ delete(statMap, "configReloadRequests")
+ delete(statMap, "configReloads")
Review comment:
Can `delete()` be inside a loop through these strings?
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
+ }
+
+ statMap := atsData.Ats
+
+ // Handle system specific values and remove them from the map for precomputing to not have issues
+ if stats.Loadavg, err = LoadavgFromRawLine(statMap["proc.loadavg"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Error parsing loadavg for cache '%s': %v", cacheName, err)
Review comment:
In Go, error messages should not be capitalized.
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
+ }
+
+ statMap := atsData.Ats
+
+ // Handle system specific values and remove them from the map for precomputing to not have issues
+ if stats.Loadavg, err = LoadavgFromRawLine(statMap["proc.loadavg"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Error parsing loadavg for cache '%s': %v", cacheName, err)
+ } else {
+ delete(statMap, "proc.loadavg")
+ }
+
+ if err := stats.AddInterfaceFromRawLine(statMap["proc.net.dev"].(string)); err != nil {
+ return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
Review comment:
In Go, error messages should not be capitalized.
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
Review comment:
Right-hand value can be just `line[delim+1:]`
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
Review comment:
Slice can be `line[delim+1:]`
##########
File path: traffic_monitor/cache/astats_csv.go
##########
@@ -0,0 +1,114 @@
+package cache
+
+/*
+ * 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.
+ */
+
+import (
+ "bufio"
+ "errors"
+ "fmt"
+ "io"
+ "strconv"
+ "strings"
+
+ "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+type astatsDataCsv struct {
+ Ats map[string]interface{}
+}
+
+func astatsCsvParseCsv(cacheName string, data io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
+ var stats Statistics
+ var err error
+ if data == nil {
+ log.Warnf("Cannot read stats data for cache '%s' - nil data reader", cacheName)
+ return stats, nil, errors.New("handler got nil reader")
+ }
+
+ var atsData astatsDataCsv
+ atsData.Ats = make(map[string]interface{})
+ scanner := bufio.NewScanner(data)
+
+ for scanner.Scan() {
+
+ line := scanner.Text()
+ delim := strings.IndexByte(line, ',')
+
+ // No delimiter found, skip this line as invalid
+ if delim < 0 {
+ continue
+ }
+ // Special cases where we just want the string value
+ if strings.Contains(line[0:delim], "proc.") || strings.Contains(line[0:delim], "inf.name") {
+ atsData.Ats[line[0:delim]] = line[delim+1 : len(line)]
+ } else {
+ value, err := strconv.ParseFloat(line[delim+1:len(line)], 64)
+
+ // Skip values that dont parse
+ if err != nil {
+ continue
+ }
+ atsData.Ats[line[0:delim]] = value
+ }
+ }
+
+ if len(atsData.Ats) < 1 {
+ return stats, nil, errors.New("No 'global' data object found in stats_over_http payload")
Review comment:
In Go, error messages should not be capitalized.
##########
File path: traffic_monitor/cache/astats.go
##########
@@ -68,50 +69,60 @@ type Astats struct {
System AstatsSystem `json:"system"`
}
-func astatsParse(cacheName string, rdr io.Reader) (Statistics, map[string]interface{}, error) {
+func astatsParse(cacheName string, rdr io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
var stats Statistics
if rdr == nil {
log.Warnf("%s handle reader nil", cacheName)
return stats, nil, errors.New("handler got nil reader")
}
- var astats Astats
- json := jsoniter.ConfigFastest
- if err := json.NewDecoder(rdr).Decode(&astats); err != nil {
- return stats, nil, err
- }
+ ctx := pollCTX.(*poller.HTTPPollCtx)
- if err := stats.AddInterfaceFromRawLine(astats.System.ProcNetDev); err != nil {
- return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
- }
- if inf, ok := stats.Interfaces[astats.System.InfName]; !ok {
- return stats, nil, errors.New("/proc/net/dev line didn't match reported interface line")
- } else {
- inf.Speed = int64(astats.System.InfSpeed)
- stats.Interfaces[astats.System.InfName] = inf
- }
+ ctype := ctx.HTTPHeader.Get("Content-Type")
- if load, err := LoadavgFromRawLine(astats.System.ProcLoadavg); err != nil {
- return stats, nil, fmt.Errorf("Failed to parse loadavg line for cache '%s': %v", cacheName, err)
- } else {
- stats.Loadavg = load
- }
+ if ctype == "text/json" || ctype == "text/javascript" || ctype == "" {
+ var astats Astats
+ json := jsoniter.ConfigFastest
+ if err := json.NewDecoder(rdr).Decode(&astats); err != nil {
+ return stats, nil, err
+ }
- stats.NotAvailable = astats.System.NotAvailable
+ if err := stats.AddInterfaceFromRawLine(astats.System.ProcNetDev); err != nil {
+ return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
Review comment:
This seems like as good of a time as ever to remove the capitalization from this error message.
##########
File path: traffic_monitor/cache/stats_over_http.go
##########
@@ -19,17 +19,19 @@ package cache
* under the License.
*/
-import "errors"
-import "fmt"
-import "io"
-import "math"
-import "strings"
-import "strconv"
+import (
+ "errors"
+ "fmt"
+ "io"
+ "math"
+ "strconv"
+ "strings"
-import "github.com/apache/trafficcontrol/lib/go-log"
-import "github.com/apache/trafficcontrol/traffic_monitor/todata"
+ "github.com/apache/trafficcontrol/lib/go-log"
-import "github.com/json-iterator/go"
+ "github.com/apache/trafficcontrol/traffic_monitor/todata"
+ jsoniter "github.com/json-iterator/go"
+)
Review comment:
These imports should be split into (at least) 3 groups:
- core package imports
- ATC imports
- external library imports
##########
File path: traffic_monitor/cache/astats.csv
##########
@@ -0,0 +1,527 @@
+proxy.process.http.completed_requests,26220072200
Review comment:
If a license header is added to this file, we don't need to re-add a `.csv` exception to `.dependency_license`, right?
##########
File path: traffic_monitor/cache/astats.go
##########
@@ -68,50 +69,60 @@ type Astats struct {
System AstatsSystem `json:"system"`
}
-func astatsParse(cacheName string, rdr io.Reader) (Statistics, map[string]interface{}, error) {
+func astatsParse(cacheName string, rdr io.Reader, pollCTX interface{}) (Statistics, map[string]interface{}, error) {
var stats Statistics
if rdr == nil {
log.Warnf("%s handle reader nil", cacheName)
return stats, nil, errors.New("handler got nil reader")
}
- var astats Astats
- json := jsoniter.ConfigFastest
- if err := json.NewDecoder(rdr).Decode(&astats); err != nil {
- return stats, nil, err
- }
+ ctx := pollCTX.(*poller.HTTPPollCtx)
- if err := stats.AddInterfaceFromRawLine(astats.System.ProcNetDev); err != nil {
- return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
- }
- if inf, ok := stats.Interfaces[astats.System.InfName]; !ok {
- return stats, nil, errors.New("/proc/net/dev line didn't match reported interface line")
- } else {
- inf.Speed = int64(astats.System.InfSpeed)
- stats.Interfaces[astats.System.InfName] = inf
- }
+ ctype := ctx.HTTPHeader.Get("Content-Type")
- if load, err := LoadavgFromRawLine(astats.System.ProcLoadavg); err != nil {
- return stats, nil, fmt.Errorf("Failed to parse loadavg line for cache '%s': %v", cacheName, err)
- } else {
- stats.Loadavg = load
- }
+ if ctype == "text/json" || ctype == "text/javascript" || ctype == "" {
+ var astats Astats
+ json := jsoniter.ConfigFastest
+ if err := json.NewDecoder(rdr).Decode(&astats); err != nil {
+ return stats, nil, err
+ }
- stats.NotAvailable = astats.System.NotAvailable
+ if err := stats.AddInterfaceFromRawLine(astats.System.ProcNetDev); err != nil {
+ return stats, nil, fmt.Errorf("Failed to parse interface line for cache '%s': %v", cacheName, err)
+ }
+ if inf, ok := stats.Interfaces[astats.System.InfName]; !ok {
+ return stats, nil, errors.New("/proc/net/dev line didn't match reported interface line")
+ } else {
+ inf.Speed = int64(astats.System.InfSpeed)
+ stats.Interfaces[astats.System.InfName] = inf
+ }
+
+ if load, err := LoadavgFromRawLine(astats.System.ProcLoadavg); err != nil {
+ return stats, nil, fmt.Errorf("Failed to parse loadavg line for cache '%s': %v", cacheName, err)
+ } else {
+ stats.Loadavg = load
+ }
- // TODO: what's using these?? Can we get rid of them?
- astats.Ats["system.astatsLoad"] = float64(astats.System.AstatsLoad)
- astats.Ats["system.configReloadRequests"] = float64(astats.System.ConfigLoadRequest)
- astats.Ats["system.configReloads"] = float64(astats.System.ConfigReloads)
- astats.Ats["system.inf.name"] = astats.System.InfName
- astats.Ats["system.inf.speed"] = float64(astats.System.InfSpeed)
- astats.Ats["system.lastReload"] = float64(astats.System.LastReload)
- astats.Ats["system.lastReloadRequest"] = float64(astats.System.LastReloadRequest)
- astats.Ats["system.notAvailable"] = stats.NotAvailable
- astats.Ats["system.proc.loadavg"] = astats.System.ProcLoadavg
- astats.Ats["system.proc.net.dev"] = astats.System.ProcNetDev
+ stats.NotAvailable = astats.System.NotAvailable
- return stats, astats.Ats, nil
+ // TODO: what's using these?? Can we get rid of them?
+ astats.Ats["system.astatsLoad"] = float64(astats.System.AstatsLoad)
+ astats.Ats["system.configReloadRequests"] = float64(astats.System.ConfigLoadRequest)
+ astats.Ats["system.configReloads"] = float64(astats.System.ConfigReloads)
+ astats.Ats["system.inf.name"] = astats.System.InfName
+ astats.Ats["system.inf.speed"] = float64(astats.System.InfSpeed)
+ astats.Ats["system.lastReload"] = float64(astats.System.LastReload)
+ astats.Ats["system.lastReloadRequest"] = float64(astats.System.LastReloadRequest)
+ astats.Ats["system.notAvailable"] = stats.NotAvailable
+ astats.Ats["system.proc.loadavg"] = astats.System.ProcLoadavg
+ astats.Ats["system.proc.net.dev"] = astats.System.ProcNetDev
+
+ return stats, astats.Ats, nil
+ } else if ctype == "text/csv" {
+ return astatsCsvParseCsv(cacheName, rdr, pollCTX)
+ } else {
+ return stats, nil, fmt.Errorf("Stats Content-Type (%s) can not be parsed by astats", ctype)
Review comment:
In Go, error messages should not be capitalized.
##########
File path: traffic_monitor/cache/stats_types.go
##########
@@ -105,7 +105,7 @@ type StatsDecoder struct {
// whatever miscellaneous data was in the payload but not represented by
// the properties of a Statistics object, so that it can be used in later
// calculations if necessary.
-type StatisticsParser func(string, io.Reader) (Statistics, map[string]interface{}, error)
+type StatisticsParser func(string, io.Reader, interface{}) (Statistics, map[string]interface{}, error)
Review comment:
It looks like this is always typecast to `*poller.HTTPPollCtx` type. Any reason we can't skip the typecast and make this `*poller.HTTPPollCtx` directly (though I understand, separately, that `handler.Handler` uses `interface{}` type to avoid an import cycle)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mattjackson220 merged pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
mattjackson220 merged pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 edited a comment on pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 edited a comment on pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#issuecomment-686070547
> > * [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
>
> Since this is a new feature, would you please add a changelog entry?
@zrhoffman Opened https://github.com/apache/trafficcontrol/pull/5009
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#issuecomment-683911396
Update to use new allocation method to do the data map allocation all up front, turns out this is more performant reading all of the string data in at once to one dynamically allocated block and then basing the size of the map on that so that the map does not have to be continually resized
BenchmarkAstatsJson-8 5836 197696 ns/op 134473 B/op 2028 allocs/op
BenchmarkAstatsCSV-8 10000 110413 ns/op 110065 B/op 800 allocs/op
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480211422
##########
File path: traffic_monitor/cache/stats_types.go
##########
@@ -105,7 +105,7 @@ type StatsDecoder struct {
// whatever miscellaneous data was in the payload but not represented by
// the properties of a Statistics object, so that it can be used in later
// calculations if necessary.
-type StatisticsParser func(string, io.Reader) (Statistics, map[string]interface{}, error)
+type StatisticsParser func(string, io.Reader, interface{}) (Statistics, map[string]interface{}, error)
Review comment:
I left it as interface{} since this is the generic definition of a parser, it gets used by any potential stats parser plugins. So specifying it as httppoll ties peoples hands as to what a plugin would be able to accept. So I think its best to leave it as generic as possible
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ezelkow1 commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480420666
##########
File path: traffic_monitor/cache/astats_test.go
##########
@@ -60,14 +57,11 @@ func TestAstatsCSV(t *testing.T) {
pl := &poller.HTTPPollCtx{HTTPHeader: http.Header{}}
ctx := interface{}(pl)
ctx.(*poller.HTTPPollCtx).HTTPHeader.Set("Content-Type", "text/csv")
- _, thismap, err := astatsParse("testCache", file, ctx)
+ _, _, err = astatsParse("testCache", file, ctx)
Review comment:
That being said I could add some value checks in the map just to test for existence, but Id prefer to do that in the next PR. I plan on working on a follow up directly after this is merged to add similar support to stats_over_http, but that is dependent on this baseline being in since it needs the Accept support and changes to allow the contexts to be passed around
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#issuecomment-686059563
> * [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
Since this is a new feature, would you please add a changelog entry?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4993: Adds support for CSV parsing in astats
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4993:
URL: https://github.com/apache/trafficcontrol/pull/4993#discussion_r480201937
##########
File path: traffic_monitor/cache/astats_test.go
##########
@@ -20,28 +20,99 @@ package cache
*/
import (
+ "bytes"
"io/ioutil"
"math/rand"
+ "net/http"
+ "os"
"testing"
"github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/traffic_monitor/poller"
"github.com/apache/trafficcontrol/traffic_monitor/todata"
-
- "github.com/json-iterator/go"
)
-func TestAstats(t *testing.T) {
- text, err := ioutil.ReadFile("astats.json")
+func TestAstatsJson(t *testing.T) {
+ file, err := os.Open("astats.json")
if err != nil {
t.Fatal(err)
}
- aStats := Astats{}
- json := jsoniter.ConfigFastest
- err = json.Unmarshal(text, &aStats)
+
+ pl := &poller.HTTPPollCtx{HTTPHeader: http.Header{}}
+ ctx := interface{}(pl)
+ ctx.(*poller.HTTPPollCtx).HTTPHeader.Set("Content-Type", "text/json")
+ _, thismap, err := astatsParse("testCache", file, ctx)
+
if err != nil {
t.Error(err)
}
- t.Logf("Found %v key/val pairs in ats\n", len(aStats.Ats))
+
+ t.Logf("Found %v key/val pairs in ats\n", len(thismap))
+
+}
+
+func TestAstatsCSV(t *testing.T) {
+ file, err := os.Open("astats.csv")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ pl := &poller.HTTPPollCtx{HTTPHeader: http.Header{}}
+ ctx := interface{}(pl)
+ ctx.(*poller.HTTPPollCtx).HTTPHeader.Set("Content-Type", "text/csv")
+ _, thismap, err := astatsParse("testCache", file, ctx)
+
+ if err != nil {
+ t.Error(err)
+ }
+
+ t.Logf("Found %v key/val pairs in ats\n", len(thismap))
Review comment:
Maybe assert the number of key/val pairs the test is supposed to find?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org