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

Rewrite current stats from Perl to Golang#4114

Merged
ocket8888 merged 7 commits intoapache:masterfrom
mhoppa:feature/rewrite_current_stats
Dec 2, 2019
Merged

Rewrite current stats from Perl to Golang#4114
ocket8888 merged 7 commits intoapache:masterfrom
mhoppa:feature/rewrite_current_stats

Conversation

@mhoppa
Copy link
Copy Markdown
Contributor

@mhoppa mhoppa commented Nov 14, 2019

What does this PR (Pull Request) do?

This PR rewrites current_stats from Perl to Golang and adds API documentation for it.

Currently no tests as we do not have Traffic Stats available for TO Api tests.

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Golang Control Client
  • Traffic Ops

What is the best way to verify this PR?

Build traffic ops with PR code and then hit /current_stats, it should return the same data as perl implementation

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

@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Nov 14, 2019

Still need to look into why Perl implementation did https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/lib/Utils/Helper.pm#L39 on each cdn name for the query and if that escaping is handled for us in the Golang Influxdb client

@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Nov 14, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4780/

@ocket8888 ocket8888 added this to the Go Rewrite milestone Nov 14, 2019
@ocket8888 ocket8888 added tech debt rework due to choosing easy/limited solution TO Client (Go) related to the Go implementation of a TC client Traffic Ops related to Traffic Ops Traffic Stats related to Traffic Stats labels Nov 14, 2019
@ocket8888 ocket8888 self-assigned this Nov 14, 2019
@ocket8888
Copy link
Copy Markdown
Contributor

So then is a work in progress or ready for review?

@mhoppa
Copy link
Copy Markdown
Contributor Author

mhoppa commented Nov 15, 2019

@ocket8888 this is ready for review. The perl code did replace on ,',",\n in the CDN names prior to going to influxdb but all are blocked from even being apart of a CDN name in the first place -> https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/cdn/cdns.go#L112

Comment thread traffic_ops/traffic_ops_golang/routing/routes.go Outdated
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go Outdated
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go Outdated
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go
Comment thread docs/source/api/current_stats.rst
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go
Comment thread traffic_ops/traffic_ops_golang/trafficstats/cdn.go Outdated
@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Nov 21, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4808/

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Appears to work fine in its own right, but has a regression, unfortunately.

Comment thread lib/go-tc/traffic_stats.go Outdated
@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Dec 2, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4847/

@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Dec 2, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4848/

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Works, docs build without error/warning and look accurate, thorough and properly rendered, all tests pass.

@ocket8888 ocket8888 merged commit 752ee5a into apache:master Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tech debt rework due to choosing easy/limited solution TO Client (Go) related to the Go implementation of a TC client Traffic Ops related to Traffic Ops Traffic Stats related to Traffic Stats

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rewrite /current_stats to Go

5 participants