Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

debug: use the new metrics stream in debug command #10399

Merged
merged 5 commits into from
Jul 27, 2021
Merged

debug: use the new metrics stream in debug command #10399

merged 5 commits into from
Jul 27, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Jun 14, 2021 •
edited

Related to #10320

Tested by running a dev agent and collecting a consul debug archive. All the metrics are indent formatted in the metrics.json.

TODO:

  • changelog
  • manual test to see what the metrics look like in the debug archive
  • add a test for the API endpoint
  • verify and improve the assertions in the debug CLI test for the metrics json file.

@github-actions github-actions bot added pr/dependencies PR specifically updates dependencies of project theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface labels Jun 14, 2021
dhiaayachi
dhiaayachi reviewed Jun 15, 2021
View reviewed changes
Copy link
Collaborator

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

I saw that this touch the debug part and I took a look, I really like how the stream API work. It make it easier to handle long running requests. As commented below I think we should also change the log API to work the same way.
I also had few comments/questions

agent/agent_endpoint.go Show resolved Hide resolved
command/debug/debug.go Outdated Show resolved Hide resolved
command/debug/debug.go Show resolved Hide resolved
agent/agent_endpoint.go Show resolved Hide resolved
@dnephin dnephin force-pushed the dnephin/debug-stream-metrics branch from 460408a to 26e7c37 Compare July 14, 2021 22:33
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 14, 2021 22:33 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 14, 2021 22:33 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 14, 2021 22:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 14, 2021 22:53 Inactive
@dnephin
Copy link
Contributor Author

dnephin commented Jul 14, 2021

I think the TODOs are resolved, and this should be ready for review

eculver
eculver reviewed Jul 14, 2021
View reviewed changes
Copy link
Contributor

@eculver eculver 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. I just had one question about an edge-case. Also, can we add a couple tests for this new endpoint?

agent/agent_endpoint.go Outdated Show resolved Hide resolved
@dnephin
Copy link
Contributor Author

dnephin commented Jul 15, 2021

can we add a couple tests for this new endpoint?

Ya, good idea. The change itself is covered by the tests for the CLI, although I could probably improve the checks in that test as well. The underlying behaviour is tested in the go-metrics library, but a test of the Consul integration wouldn't hurt.

@eculver eculver mentioned this pull request Jul 16, 2021
Merged
@dnephin
Update armon/go-metrics
5f753de 
To pickup new InMemSink.Stream method
@dnephin dnephin force-pushed the dnephin/debug-stream-metrics branch from fc24d64 to 421e23d Compare July 26, 2021 21:52
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 26, 2021 21:52 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 26, 2021 21:52 Inactive
@dnephin dnephin force-pushed the dnephin/debug-stream-metrics branch from 421e23d to e58a074 Compare July 26, 2021 21:54
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 26, 2021 21:54 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 26, 2021 21:54 Inactive
@dnephin
Copy link
Contributor Author

dnephin commented Jul 26, 2021

Ok, I've rebased this PR and added a test for the HTTP endpoint, and improved the test coverage of the CLI.

I think this is ready for another review.

eculver
eculver approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@eculver eculver left a comment

Choose a reason for hiding this comment

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

software of the highest quality.. plus tests!

@dnephin dnephin merged commit a0b1149 into main Jul 27, 2021
@dnephin dnephin deleted the dnephin/debug-stream-metrics branch July 27, 2021 17:23
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/416873.

@dnephin dnephin mentioned this pull request Aug 6, 2021
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eculver eculver eculver approved these changes

@dhiaayachi dhiaayachi Awaiting requested review from dhiaayachi

Assignees
No one assigned
Labels
pr/dependencies PR specifically updates dependencies of project theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dnephin @hc-github-team-consul-core @eculver @dhiaayachi