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

Add csv support to stats_over_http#5023

Merged
zrhoffman merged 5 commits intoapache:masterfrom
ezelkow1:tm-csv-stats
Oct 5, 2020
Merged

Add csv support to stats_over_http#5023
zrhoffman merged 5 commits intoapache:masterfrom
ezelkow1:tm-csv-stats

Conversation

@ezelkow1
Copy link
Member

What does this PR (Pull Request) do?

This uses the same parsing method as astats csv and provides almost double the performance with less than half the allocations of the json version.

Also add a null check for dsStats. Its possible to do a request to ATS to a DS that does not exist, ATS will then insert stats for that request into the remap stats list. When it gets to TM though it will not be in the list of known DSs so if we try to index by it it will end up null

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

Which Traffic Control components are affected by this PR?

  • Traffic Monitor

What is the best way to verify this PR?

  • Add health.polling.format = stats_over_http to a server profile
  • Set http_polling_format for TM to be "text/csv"
  • Check if stats are pulled correctly in TM

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

Benchmarks comparing all stats parsing, both astats and stats over http with json and csv. In both CSV instances they are ~100% faster with less than half as many allocations

BenchmarkAstatsJson-8 5276 202209 ns/op 134453 B/op 2028 allocs/op
BenchmarkAstatsCSV-8 9963 111798 ns/op 110001 B/op 798 allocs/op
BenchmarkStatsJson-8 5512 227104 ns/op 134991 B/op 2082 allocs/op
BenchmarkStatsCSV-8 10000 108320 ns/op 111217 B/op 837 allocs/op

ezelkow1 and others added 2 commits September 11, 2020 11:58
This uses the same parsing method as astats csv and provides almost double the performance with less than half the allocations of the json version.

Also add a null check for dsStats. Its possible to do a request to ATS to a DS that does not exist, ATS will then insert stats for that request into the remap stats list. When it gets to TM though it will not be in the list of known DSs so if we try to index by it it will end up null
@zrhoffman zrhoffman added new feature A new feature, capability or behavior tests related to tests and/or testing infrastructure Traffic Monitor related to Traffic Monitor labels Sep 14, 2020
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.

Our docs do not mention that, in order for Traffic Monitor to successfully poll servers using stats_over_http output, the experimental system_stats ATS plugin must also been enabled.

Otherwise, you get an unhelpful error like REPORTED - Error parsing loadavg for cache 'edge': Data was missing 'plugin.system_stats.loadavg.one'; eth0: not found in polled data.

So, this is a good time to make sure our documentation includes that info.

* Remove my dstat nil check since this has been merged in already
…SoH and that it can be used to provide system info for any custom extensions
@ezelkow1
Copy link
Member Author

ezelkow1 commented Oct 5, 2020

Our docs do not mention that, in order for Traffic Monitor to successfully poll servers using stats_over_http output, the experimental system_stats ATS plugin must also been enabled.

Otherwise, you get an unhelpful error like REPORTED - Error parsing loadavg for cache 'edge': Data was missing 'plugin.system_stats.loadavg.one'; eth0: not found in polled data.

So, this is a good time to make sure our documentation includes that info.

Thanks

Updated changelog, removed nil check since it went in to the codebase before this went through, and put info in the docs about adding system_stats

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.

Looks good!

  • TM successfully reports cache health when when accepting text/csv polling responses. See the CDN-in-a-Box CI result from https://github.com/zrhoffman/trafficcontrol/commits/tm-csv-stats, which uses ATS 9 and stats_over_http with CSV polling
  • Code, documentation, and changelog look good
  • Tests and benchmarks pass and have good coverage

@zrhoffman zrhoffman merged commit 33bb154 into apache:master Oct 5, 2020
@ezelkow1 ezelkow1 deleted the tm-csv-stats branch October 5, 2020 19:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new feature A new feature, capability or behavior tests related to tests and/or testing infrastructure Traffic Monitor related to Traffic Monitor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants