Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Adds support for CSV parsing in astats#4993

Merged
mattjackson220 merged 4 commits intoapache:masterfrom
ezelkow1:tm-csv
Sep 2, 2020
Merged

Adds support for CSV parsing in astats#4993
mattjackson220 merged 4 commits intoapache:masterfrom
ezelkow1:tm-csv

Conversation

@ezelkow1
Copy link
Member

@ezelkow1 ezelkow1 commented Aug 27, 2020

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.

  • This PR fixes #REPLACE_ME OR is not related to any Issue

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Monitor

What is the best way to verify this PR?

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?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • 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
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

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.

Updating with new benchmark numbers after changing to use upfront allocation on the data map:
BenchmarkAstatsJson-8 5836 197696 ns/op 134473 B/op 2028 allocs/op
BenchmarkAstatsCSV-8 10000 110413 ns/op 110065 B/op 800 allocs/op

The csv is now more efficient both for operations and allocations compared to json doing a single up front allocation of the map based on the number of lines returned from astats

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.
Now scans in all data in to a string slice which then lets us allocate the data map up front.  In benches this bumps the performance up from ~8000 to 10000
@ezelkow1
Copy link
Member Author

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

Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TM tests pass
  • TM successfully parses CSV polling responses and reports stats correctly
  • Nothing important is on the CSV lines that fail to parse and are ignored
  • Docs build and look good

@mattjackson220 mattjackson220 merged commit a3fde2e into apache:master Sep 2, 2020
@mattjackson220 mattjackson220 added documentation related to documentation new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor labels Sep 2, 2020
@zrhoffman
Copy link
Member

  • 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?

@ezelkow1
Copy link
Member Author

ezelkow1 commented Sep 2, 2020

  • 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 #5009

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation related to documentation new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants