-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[receiver/splunkhecreceiver] make splunk_hec receiver healthcheck mimic the real Splunk HEC behaviour #20873
[receiver/splunkhecreceiver] make splunk_hec receiver healthcheck mimic the real Splunk HEC behaviour #20873
Conversation
Foresight Summary
View More Details✅ check-links workflow has finished in 54 seconds and finished at 13th Apr, 2023.
✅ telemetrygen workflow has finished in 1 minute 1 second and finished at 13th Apr, 2023.
✅ prometheus-compliance-tests workflow has finished in 3 minutes 5 seconds (3 minutes 19 seconds less than
|
Job | Failed Steps | Tests | |
---|---|---|---|
prometheus-compliance-tests | - 🔗 | N/A | See Details |
✅ e2e-tests workflow has finished in 14 minutes 17 seconds and finished at 13th Apr, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
kubernetes-test (v1.26.0) | - 🔗 | N/A | See Details |
kubernetes-test (v1.25.3) | - 🔗 | N/A | See Details |
kubernetes-test (v1.24.7) | - 🔗 | N/A | See Details |
kubernetes-test (v1.23.13) | - 🔗 | N/A | See Details |
✅ load-tests workflow has finished in 16 minutes 1 second (⚠️ 5 minutes 28 seconds more than main
branch avg.) and finished at 13th Apr, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
setup-environment | - 🔗 | N/A | See Details |
loadtest (TestIdleMode) | - 🔗 | N/A | See Details |
loadtest (TestBallastMemory|TestLog10kDPS) | - 🔗 | N/A | See Details |
loadtest (TestMetric10kDPS|TestMetricsFromFile) | - 🔗 | N/A | See Details |
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) | - 🔗 | N/A | See Details |
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) | - 🔗 | N/A | See Details |
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) | - 🔗 | N/A | See Details |
loadtest (TestTraceAttributesProcessor) | - 🔗 | N/A | See Details |
✅ build-and-test workflow has finished in 35 minutes 26 seconds (11 minutes 16 seconds less than main
branch avg.) and finished at 13th Apr, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
govulncheck | - 🔗 | N/A | See Details |
setup-environment | - 🔗 | N/A | See Details |
checks | - 🔗 | N/A | See Details |
check-collector-module-version | - 🔗 | N/A | See Details |
lint-matrix (receiver-0) | - 🔗 | N/A | See Details |
lint-matrix (receiver-1) | - 🔗 | N/A | See Details |
lint-matrix (processor) | - 🔗 | N/A | See Details |
lint-matrix (exporter) | - 🔗 | N/A | See Details |
lint-matrix (extension) | - 🔗 | N/A | See Details |
lint-matrix (connector) | - 🔗 | N/A | See Details |
lint-matrix (internal) | - 🔗 | N/A | See Details |
lint-matrix (other) | - 🔗 | N/A | See Details |
build-examples | - 🔗 | N/A | See Details |
check-codeowners | - 🔗 | N/A | See Details |
correctness-metrics | - 🔗 | N/A | See Details |
integration-tests | - 🔗 | N/A | See Details |
correctness-traces | - 🔗 | N/A | See Details |
unittest-matrix (1.20, receiver-0) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, receiver-1) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, processor) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, exporter) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, extension) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, connector) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, internal) | - 🔗 | N/A | See Details |
unittest-matrix (1.20, other) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, receiver-0) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, receiver-1) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, processor) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, exporter) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, extension) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, connector) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, internal) | - 🔗 | N/A | See Details |
unittest-matrix (1.19, other) | - 🔗 | N/A | See Details |
lint | - 🔗 | N/A | See Details |
unittest (1.20) | - 🔗 | N/A | See Details |
unittest (1.19) | - 🔗 | N/A | See Details |
cross-compile (darwin, amd64) | - 🔗 | N/A | See Details |
cross-compile (darwin, arm64) | - 🔗 | N/A | See Details |
cross-compile (linux, 386) | - 🔗 | N/A | See Details |
cross-compile (linux, amd64) | - 🔗 | N/A | See Details |
cross-compile (linux, arm) | - 🔗 | N/A | See Details |
cross-compile (linux, arm64) | - 🔗 | N/A | See Details |
cross-compile (linux, ppc64le) | - 🔗 | N/A | See Details |
cross-compile (windows, 386) | - 🔗 | N/A | See Details |
cross-compile (windows, amd64) | - 🔗 | N/A | See Details |
build-package (deb) | - 🔗 | N/A | See Details |
build-package (rpm) | - 🔗 | N/A | See Details |
windows-msi | - 🔗 | N/A | See Details |
publish-check | - 🔗 | N/A | See Details |
publish-stable | - 🔗 | N/A | See Details |
publish-dev | - 🔗 | N/A | See Details |
rotate-milestone | - 🔗 | N/A | See Details |
⭕ build-and-test-windows workflow has finished in 3 seconds (30 minutes 31 seconds less than main
branch avg.) and finished at 13th Apr, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
windows-unittest-matrix | - 🔗 | N/A | See Details |
windows-unittest | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 2 minutes 11 seconds and finished at 13th Apr, 2023.
Job | Failed Steps | Tests | |
---|---|---|---|
changelog | - 🔗 | N/A | See Details |
*You can configure Foresight comments in your organization settings page.
@omrozowicz-splunk the 2 responses in the description look the same to me except for some spacing. Is the spacing the issue or is there a typo in the description? |
writer.Header().Add("Content-Type", "application/json") | ||
writer.WriteHeader(http.StatusOK) | ||
_, err := writer.Write([]byte(responseHecHealthy)) | ||
if err != nil { |
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.
Since we aren't doing anything with this error I think we can ignore its return from the Write
call with a _
.
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.
Yes, although my IDE shows "Unhandled error" warnings, I think It doesn't make sense as the value is always the same and hardcoded. I replace this line with bare writer.Write([]byte(responseHecHealthy))
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.
Ok, linter failed, you were right. Now it is _, _ = writer.Write([]byte(responseHecHealthy))
@@ -1042,12 +1042,12 @@ func Test_splunkhecreceiver_handleHealthPath(t *testing.T) { | |||
assert.NoError(t, r.Shutdown(context.Background())) | |||
}() | |||
w := httptest.NewRecorder() | |||
r.handleHealthReq(w, httptest.NewRequest("POST", "http://localhost/services/collector/health", nil)) | |||
r.handleHealthReq(w, httptest.NewRequest("GET", "http://localhost/services/collector/health", nil)) |
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.
it's fine to test GET only.
@@ -202,7 +203,8 @@ func (r *splunkReceiver) Start(_ context.Context, host component.Host) error { | |||
} | |||
|
|||
mx := mux.NewRouter() | |||
mx.NewRoute().Path(r.config.HealthPath).HandlerFunc(r.handleHealthReq) | |||
mx.NewRoute().Path(r.config.HealthPath).HandlerFunc(r.handleHealthReq).Methods("GET") | |||
mx.NewRoute().Path(r.config.HealthPath + "/1.0").HandlerFunc(r.handleHealthReq).Methods("GET") |
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.
we can limit this one to GET.
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.
Ok, /1.0
one I left unchanged
Sorry, I phrased it wrongly. I meant to show that now these responses are the same. You can check the current behaviour in the issue I linked. |
Description: Make Splunk HEC endpoint mimic the real one. After applying changes from this PR the response from the Splunk HEC health endpoint of Splunk HEC receiver will be:
Which is the same for the real Splunk HEC:
The same for
services/collector/health/1.0
endpoint.For
/1.0
support I just concatenate healthPath + "/1.0", not sure if it is a correct approach when user sets up its own healthPath endpoint. An alternative here is to create another variable or to create /1.0 route only when healthPath is the default one.Also this endpoint returns a proper json, not the plain text response like the rest of our endpoints (for ex.
"{\"text\":\"No data\",\"code\":5}"
)I limited this endpoint to accept only
GET
method as this is how it works in the real Splunk HEC.Link to tracking Issue: 20871
Testing: Unit tests, manual tests
Documentation: Nothing new added