Adding http.status_code as span attributes for both Apache/Nginx #235
Conversation
| @@ -47,7 +47,6 @@ class RequestPayload | |||
| std::string flavor; | |||
| std::string client_ip; | |||
| long port = 80; | |||
| long status_code = 200; | |||
There was a problem hiding this comment.
No magic numbers here? We can find a better way right?
There was a problem hiding this comment.
This is a removed code.
| @@ -1101,18 +1101,18 @@ static void stopMonitoringRequest(ngx_http_request_t* r, | |||
| } | |||
|
|
|||
| APPD_SDK_STATUS_CODE res; | |||
| unsigned int errCode=0; | |||
There was a problem hiding this comment.
Same as above..Lets use a better practice
There was a problem hiding this comment.
This is unsigned int initialised to zero value. Do you think we really need to define this else where
| char* msg = NULL; | ||
|
|
||
| if (otel_requestHasErrors(r)) | ||
| { | ||
| errCode=(unsigned int)otel_getErrorCode(r); | ||
| res_payload->status_code = (unsigned int)otel_getErrorCode(r); | ||
| msg = (char*)malloc(6); |
There was a problem hiding this comment.
Please replace fixed number 6 by a constant
| @@ -131,7 +136,6 @@ TEST(TestRequestProcessingEngine, StartRequest) | |||
| payload.set_request_protocol("GET"); | |||
| payload.set_uri("dummy_span"); | |||
| payload.set_server_name("localhost"); | |||
| payload.set_status_code(200); | |||
There was a problem hiding this comment.
Same as above can't use magic numbers
instrumentation/otel-webserver-module/test/unit/RequestProcessingEngine_test.cpp
Show resolved
Hide resolved
instrumentation/otel-webserver-module/test/unit/RequestProcessingEngine_test.cpp
Show resolved
Hide resolved
| @@ -229,12 +232,19 @@ TEST(TestRequestProcessingEngine, EndRequest) | |||
| EXPECT_CALL(*getMockSpan(interactionSpan1), End()). | |||
| Times(1); | |||
|
|
|||
| unsigned int status_code = 403; | |||
There was a problem hiding this comment.
These are test code though. We are testing with a value. Do you really think we need to avoid magic number
|
|
||
| EXPECT_CALL(*getMockSpan(rootSpan), End()). | ||
| Times(1); | ||
|
|
||
| auto res = engine.endRequest(rContext, "403"); |
There was a problem hiding this comment.
String literals should go into a resource file ..outside the code
marcalff
added
the
Webserver
No description provided.