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

Handle HTTP errors from metrics API #23

Merged
merged 3 commits into from
Nov 27, 2019
Merged

Handle HTTP errors from metrics API #23

merged 3 commits into from
Nov 27, 2019

Conversation

pda
Copy link
Member

@pda pda commented Nov 22, 2019

buildkite.Client.GetAgentMetrics() was not handling HTTP errors, and instead returning an empty AgentMetrics struct.

This manifested as an apparent zero-value for all metrics, and an empty org field in the log output:

2019/11/21 19:38:23 Publishing metric WaitingJobsCount=0 [org=,queue=default]

Go's HTTP client says:

A non-2xx status code doesn't cause an error.
-- https://golang.org/pkg/net/http/#Client.Do

This PR adds status-checking code, and tests for the happy and sad cases.

@pda
buildkite.queryMetrics: defer res.Body.Close() as early as possible
291fae7
@pda
buildkite.queryMetrics: extract method var for near-future use
f652479
@pda
buildkite.Client treats non-200 HTTP status as error
530d902 
Previously, error responses such as 401 Unauthorized were not treated as
errors, and instead an empty response payload was returned, resulting in
mysterious behaviour.
@pda pda requested a review from lox November 22, 2019 05:03
lox
lox approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@lox lox left a comment

Choose a reason for hiding this comment

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

Super legit. 👌🏻

@pda pda merged commit 565b2b3 into master Nov 27, 2019
@pda pda deleted the handle-http-error branch November 27, 2019 00:45
@pda pda mentioned this pull request Aug 17, 2020
Merged
@yob yob mentioned this pull request Sep 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lox lox lox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pda @lox