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

HttpServerConnection: always log request filter if any, for debugging #9968

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HttpServerConnection: always log request filter if any, for debugging #9968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/remote/httphandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ void HttpHandler::ProcessRequest(
AsioTlsStream& stream,
const ApiUser::Ptr& user,
boost::beast::http::request<boost::beast::http::string_body>& request,
Url::Ptr& url,
Dictionary::Ptr& params,
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

At least those should be clearly marked as output parameters, at least from the signature it's not really clear why user is an input parameter but url and params are not. For response, on can at least argue that this seems likely that a function ProcessRequest() generates a response.

But in general, a version where such indirect passing around of values wasn't needed at all might be preferably, maybe by doing the parameter evaluation where it's needed or moving more specific logging to the inside where more context is available (and only logging errors in the existing place).

Maybe something like a more explicit "extra values to be logged" output parameter could be an option. That could also be set to the concrete relevant values when they are actually used rather then extracting them separately in the logging code.

Copy link
Member Author

Choose a reason for hiding this comment

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

A matter of taste I'd say. I prefer to keep all the log stuff in that place.

boost::beast::http::response<boost::beast::http::string_body>& response,
boost::asio::yield_context& yc,
HttpServerConnection& server
Expand All @@ -58,7 +60,7 @@ void HttpHandler::ProcessRequest(
Dictionary::Ptr node = m_UrlTree;
std::vector<HttpHandler::Ptr> handlers;

Url::Ptr url = new Url(std::string(request.target()));
url = new Url(std::string(request.target()));
auto& path (url->GetPath());

for (std::vector<String>::size_type i = 0; i <= path.size(); i++) {
Expand Down Expand Up @@ -89,8 +91,6 @@ void HttpHandler::ProcessRequest(

std::reverse(handlers.begin(), handlers.end());

Dictionary::Ptr params;

try {
params = HttpUtility::FetchRequestParameters(url, request.body());
} catch (const std::exception& ex) {
Expand Down
2 changes: 2 additions & 0 deletions lib/remote/httphandler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class HttpHandler : public Object
AsioTlsStream& stream,
const ApiUser::Ptr& user,
boost::beast::http::request<boost::beast::http::string_body>& request,
Url::Ptr& url,
Dictionary::Ptr& params,
boost::beast::http::response<boost::beast::http::string_body>& response,
boost::asio::yield_context& yc,
HttpServerConnection& server
Expand Down
47 changes: 44 additions & 3 deletions lib/remote/httpserverconnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,9 @@ static inline
bool ProcessRequest(
AsioTlsStream& stream,
boost::beast::http::request<boost::beast::http::string_body>& request,
Url::Ptr& url,
ApiUser::Ptr& authenticatedUser,
Dictionary::Ptr& params,
boost::beast::http::response<boost::beast::http::string_body>& response,
HttpServerConnection& server,
bool& hasStartedStreaming,
Expand All @@ -454,7 +456,7 @@ bool ProcessRequest(
try {
CpuBoundWork handlingRequest (yc);

HttpHandler::ProcessRequest(stream, authenticatedUser, request, response, yc, server);
HttpHandler::ProcessRequest(stream, authenticatedUser, request, url, params, response, yc, server);
} catch (const std::exception& ex) {
if (hasStartedStreaming) {
return false;
Expand Down Expand Up @@ -542,16 +544,55 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc)
authenticatedUser = ApiUser::GetByAuthHeader(std::string(request[http::field::authorization]));
}

Url::Ptr url;
Dictionary::Ptr params;
Log logMsg (LogInformation, "HttpServerConnection");

logMsg << "Request " << request.method_string() << ' ' << request.target()
<< " (from " << m_PeerAddress
<< "), user: " << (authenticatedUser ? authenticatedUser->GetName() : "<unauthenticated>")
<< ", agent: " << request[http::field::user_agent]; //operator[] - Returns the value for a field, or "" if it does not exist.

Defer addRespCode ([&response, start, &logMsg]() {
Defer completeLogMessage ([&response, start, &authenticatedUser, &url, &params, &logMsg]() {
logMsg << ", status: " << response.result() << ") took "
<< ch::duration_cast<ch::milliseconds>(ch::steady_clock::now() - start).count() << "ms.";

if (authenticatedUser && params) {
auto filter (HttpUtility::GetLastParameter(params, "filter"));

if (filter.GetType() != ValueEmpty) {
bool urlHasFilter = false;

if (url) {
for (auto& kv : url->GetQuery()) {
if (kv.first == "filter") {
urlHasFilter = true;
break;
}
}
}

if (!urlHasFilter) {
auto type (HttpUtility::GetLastParameter(params, "type"));

if (type.GetType() == ValueEmpty) {
logMsg << " Filter: ";
} else {
logMsg << ' ' << type << " filter: ";
}

String filterStr = filter;
const auto filterLimit (1024u);

if (filterStr.GetLength() > filterLimit) {
filterStr.erase(filterStr.Begin() + filterLimit, filterStr.End());
filterStr += "...";
}

logMsg << filterStr;
}
}
}
});

if (!HandleAccessControl(*m_Stream, request, response, yc)) {
Expand All @@ -572,7 +613,7 @@ void HttpServerConnection::ProcessMessages(boost::asio::yield_context yc)

m_Seen = std::numeric_limits<decltype(m_Seen)>::max();

if (!ProcessRequest(*m_Stream, request, authenticatedUser, response, *this, m_HasStartedStreaming, yc)) {
if (!ProcessRequest(*m_Stream, request, url, authenticatedUser, params, response, *this, m_HasStartedStreaming, yc)) {
break;
}

Expand Down
Loading