-
Notifications
You must be signed in to change notification settings - Fork 247
Adding code to address docker healthcheck issue #1285
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.
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
Changes from 17 commits
eff0b3b
9215a4a
3c57fe0
b41677e
f26b71c
321da87
50cc518
c89eff0
240e19c
2f36a68
6856c9c
e35fe1c
9dd4611
94e18a1
aa04ba5
37888a4
f1722b2
3157b7f
d591e62
4cb0364
07315c6
2bc7fbe
9cd838b
fb54f42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package main | ||
|
||
import ( | ||
"flag" | ||
"fmt" | ||
"log" | ||
"net/http" | ||
"os" | ||
"strconv" | ||
) | ||
|
||
// validatePort checks if the port configuration is valid | ||
func validatePort(port string) error { | ||
|
||
portInt, err := strconv.Atoi(port) | ||
if err != nil { | ||
return fmt.Errorf("invalid port: %w", err) | ||
} | ||
if portInt < 1 || portInt > 65535 { | ||
return fmt.Errorf("port outside of range [1;65535]: %d", portInt) | ||
} | ||
return nil | ||
|
||
} | ||
|
||
func main() { | ||
const host = "127.0.0.1" // default host | ||
usedPort := "13133" // default port | ||
const path = "/" //default path | ||
generateCmd := flag.NewFlagSet("generate", flag.ExitOnError) | ||
port := generateCmd.String("port", usedPort, "Specify collector health-check port") | ||
|
||
if len(os.Args) > 1 { | ||
err := generateCmd.Parse(os.Args[1:]) | ||
if err != nil { | ||
log.Fatalf("%s", err) | ||
} | ||
} | ||
|
||
validationErr := validatePort(*port) | ||
if validationErr != nil { | ||
log.Fatalf("%s", validationErr) | ||
} | ||
|
||
resp, err := http.Get(fmt.Sprint("http://", host, ":", *port, path)) | ||
if err != nil { | ||
log.Fatalf("Unable to retrieve health status: %s", err.Error()) | ||
bryan-aguilar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if resp != nil && resp.StatusCode == 200 { | ||
log.Printf("STATUS: %d", resp.StatusCode) | ||
} else { | ||
bryan-aguilar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
log.Fatalf("STATUS: %d", resp.StatusCode) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,139 @@ | ||||||
package main | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
"github.com/stretchr/testify/assert" | ||||||
"io/ioutil" | ||||||
"net" | ||||||
"net/http" | ||||||
"net/http/httptest" | ||||||
"os" | ||||||
"os/exec" | ||||||
"testing" | ||||||
) | ||||||
|
||||||
func TestHealthStatusHealthy(t *testing.T) { | ||||||
if os.Getenv("FLAG") == "1" { | ||||||
bryan-aguilar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
server := setUpMockCollector("127.0.0.1:13133", http.StatusOK) | ||||||
os.Args = []string{"-port=13133"} | ||||||
defer server.Close() | ||||||
main() | ||||||
return | ||||||
} // Run the test in a subprocess | ||||||
cmd := exec.Command(os.Args[0], "-test.run=TestHealthStatusHealthy") //nolint:gosec | ||||||
cmd.Env = append(os.Environ(), "FLAG=1") | ||||||
|
||||||
stdout, _ := cmd.StderrPipe() | ||||||
er := cmd.Start() | ||||||
if er != nil { | ||||||
t.Fatal("Unable to start the testing sub-process.") | ||||||
} | ||||||
|
||||||
gotBytes, _ := ioutil.ReadAll(stdout) | ||||||
got := string(gotBytes) | ||||||
expectedErrorString := "STATUS: 200" | ||||||
|
||||||
assert.Contains(t, got[:len(got)-1], expectedErrorString, | ||||||
fmt.Sprintf("Unexpected log message. Got %s but should contain %s", got, expectedErrorString)) | ||||||
|
||||||
err := cmd.Wait() | ||||||
e, _ := err.(*exec.ExitError) | ||||||
assert.Nil(t, e) | ||||||
|
||||||
} | ||||||
|
||||||
func TestHealthStatusUnhealthy(t *testing.T) { | ||||||
if os.Getenv("FLAG") == "1" { | ||||||
server := setUpMockCollector("127.0.0.1:13133", http.StatusInternalServerError) | ||||||
os.Args = []string{"-port=13133"} | ||||||
defer server.Close() | ||||||
main() | ||||||
return | ||||||
} // Run the test in a subprocess | ||||||
cmd := exec.Command(os.Args[0], "-test.run=TestHealthStatusUnhealthy") //nolint:gosec | ||||||
cmd.Env = append(os.Environ(), "FLAG=1") | ||||||
|
||||||
stdout, _ := cmd.StderrPipe() | ||||||
er := cmd.Start() | ||||||
if er != nil { | ||||||
t.Fatal("Unable to start the testing sub-process.") | ||||||
} | ||||||
|
||||||
gotBytes, _ := ioutil.ReadAll(stdout) | ||||||
got := string(gotBytes) | ||||||
expectedErrorString := "STATUS: 500" | ||||||
|
||||||
assert.Contains(t, got[:len(got)-1], expectedErrorString, | ||||||
fmt.Sprintf("Unexpected log message. Got %s but should contain %s", got, expectedErrorString)) | ||||||
|
||||||
err := cmd.Wait() | ||||||
expectedErrorStatus := "exit status 1" | ||||||
e, ok := err.(*exec.ExitError) | ||||||
assert.Equal(t, true, ok) | ||||||
assert.Equal(t, expectedErrorStatus, e.Error()) | ||||||
|
||||||
} | ||||||
func TestHealthStatusServerDown(t *testing.T) { | ||||||
if os.Getenv("FLAG") == "1" { | ||||||
server := setUpMockCollector("127.0.0.1:13132", http.StatusInternalServerError) | ||||||
os.Args = []string{"-port=13133"} | ||||||
defer server.Close() | ||||||
main() | ||||||
return | ||||||
} // Run the test in a subprocess | ||||||
cmd := exec.Command(os.Args[0], "-test.run=TestHealthStatusServerDown") //nolint:gosec | ||||||
cmd.Env = append(os.Environ(), "FLAG=1") | ||||||
|
||||||
stdout, _ := cmd.StderrPipe() | ||||||
er := cmd.Start() | ||||||
if er != nil { | ||||||
t.Fatal("Unable to start the testing sub-process.") | ||||||
} | ||||||
|
||||||
gotBytes, _ := ioutil.ReadAll(stdout) | ||||||
got := string(gotBytes) | ||||||
expectedErrorString := "Unable to retrieve health status" | ||||||
|
||||||
assert.Contains(t, got[:len(got)-1], expectedErrorString, | ||||||
fmt.Sprintf("Unexpected log message. Got %s but should contain %s", got, expectedErrorString)) | ||||||
|
||||||
err := cmd.Wait() | ||||||
expectedErrorStatus := "exit status 1" | ||||||
e, ok := err.(*exec.ExitError) | ||||||
assert.Equal(t, true, ok) | ||||||
assert.Equal(t, expectedErrorStatus, e.Error()) | ||||||
|
||||||
} | ||||||
|
||||||
func TestValidatePortWithWrongString(t *testing.T) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These next three test cases would be better suited off using a Table DrivenTesting format. I believe the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also add a test case for an empty string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks. Updated. |
||||||
expectedErrorString := "invalid port" | ||||||
actual := validatePort("wrongPort") | ||||||
|
||||||
assert.Contains(t, actual.Error(), expectedErrorString, | ||||||
fmt.Sprintf("Unexpected log message. Got %s but should contain %s", actual, expectedErrorString)) | ||||||
} | ||||||
|
||||||
func TestValidatePortWithWrongPort(t *testing.T) { | ||||||
expectedErrorString := "port outside of range" | ||||||
actual := validatePort("65536") | ||||||
|
||||||
assert.Contains(t, actual.Error(), expectedErrorString, | ||||||
fmt.Sprintf("Unexpected log message. Got %s but should contain %s", actual, expectedErrorString)) | ||||||
} | ||||||
|
||||||
func TestValidatePortWithValidPort(t *testing.T) { | ||||||
actual := validatePort("65535") | ||||||
|
||||||
assert.Nil(t, actual) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Updated. |
||||||
} | ||||||
|
||||||
func setUpMockCollector(healthCheckDefaultEndpoint string, statusCode int) *httptest.Server { | ||||||
l, _ := net.Listen("tcp", healthCheckDefaultEndpoint) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not ignore the error here. Instead use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not see the update here. Has the change been pushed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh.. may be it was not pushed that time. Its added on line 59. |
||||||
server := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { | ||||||
rw.WriteHeader(statusCode) | ||||||
})) | ||||||
server.Listener.Close() | ||||||
server.Listener = l | ||||||
server.Start() | ||||||
return server | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
extensions: | ||
health_check: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to specify endpoint and path in this config like the others? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have removed specific mention of endpoint and path from all configs- as pointed out by in one of the comments by Anthony. The default healthcheck endpoints will be applicable in this case. |
||
|
||
receivers: | ||
otlp: | ||
protocols: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this get utilized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per your initial comment I added build commands for other architectures..