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
Expose Peak memory usage in query statistics. #51946
Changes from 18 commits
a2c9e26
a41ec12
b276f45
ffa4f37
7e99295
6f4d6fd
bdfaffb
54838e9
901089b
3fcfbb1
09ba975
1a7ee40
7704469
feebad3
2bc7bb0
1d4d829
e24883d
68a501a
62f9a95
810137e
d4d381d
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 |
---|---|---|
|
@@ -22,6 +22,9 @@ struct ThreadEventData | |
UInt64 user_ms = 0; | ||
UInt64 system_ms = 0; | ||
UInt64 memory_usage = 0; | ||
|
||
// -1 used as flag 'is not show for old servers' | ||
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. is not show -> is not shown |
||
Int64 peak_memory_usage = -1; | ||
}; | ||
|
||
using HostToTimesMap = std::unordered_map<String, ThreadEventData>; | ||
|
@@ -64,6 +67,7 @@ class ProgressIndication | |
{ | ||
UInt64 total = 0; | ||
UInt64 max = 0; | ||
Int64 peak = -1; | ||
}; | ||
|
||
MemoryUsage getMemoryUsage() const; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ struct ProgressValues | |
|
||
void read(ReadBuffer & in, UInt64 server_revision); | ||
void write(WriteBuffer & out, UInt64 client_revision) const; | ||
void writeJSON(WriteBuffer & out) const; | ||
void writeJSON(WriteBuffer & out, bool add_braces = true) const; | ||
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. Default values are harmful. |
||
}; | ||
|
||
struct ReadProgress | ||
|
@@ -118,7 +118,7 @@ struct Progress | |
void write(WriteBuffer & out, UInt64 client_revision) const; | ||
|
||
/// Progress in JSON format (single line, without whitespaces) is used in HTTP headers. | ||
void writeJSON(WriteBuffer & out) const; | ||
void writeJSON(WriteBuffer & out, bool add_braces = true) const; | ||
|
||
/// Each value separately is changed atomically (but not whole object). | ||
bool incrementPiecewiseAtomically(const Progress & rhs); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,9 +86,16 @@ static void dumpMemoryTracker(ProfileEventsSnapshot const & snapshot, DB::Mutabl | |
columns[i++]->insert(static_cast<UInt64>(snapshot.current_time)); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.thread_id)); | ||
columns[i++]->insert(Type::GAUGE); | ||
|
||
columns[i++]->insertData(MemoryTracker::USAGE_EVENT_NAME, strlen(MemoryTracker::USAGE_EVENT_NAME)); | ||
columns[i++]->insert(snapshot.memory_usage); | ||
columns[i]->insert(snapshot.memory_usage); | ||
|
||
i = 0; | ||
columns[i++]->insertData(host_name.data(), host_name.size()); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.current_time)); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.thread_id)); | ||
columns[i++]->insert(Type::GAUGE); | ||
columns[i++]->insertData(MemoryTracker::PEAK_USAGE_EVENT_NAME, strlen(MemoryTracker::PEAK_USAGE_EVENT_NAME)); | ||
columns[i]->insert(snapshot.peak_memory_usage); | ||
} | ||
Comment on lines
+90
to
99
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. Please explain those lines to me. I do not understand it. 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. The data (ProfileEvents and memory events) between the server and the client passes through row in the sql block (DB::Block). I added one more row with my data (peak memory).
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. 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, my bad. 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 don't understand, what exactly is confusing? The first block write current_memory_usage. Insert() |
||
|
||
void getProfileEvents( | ||
|
@@ -121,6 +128,7 @@ void getProfileEvents( | |
group_snapshot.thread_id = 0; | ||
group_snapshot.current_time = time(nullptr); | ||
group_snapshot.memory_usage = thread_group->memory_tracker.get(); | ||
group_snapshot.peak_memory_usage = thread_group->memory_tracker.getPeak(); | ||
auto group_counters = thread_group->performance_counters.getPartiallyAtomicSnapshot(); | ||
auto prev_group_snapshot = last_sent_snapshots.find(0); | ||
group_snapshot.counters = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
#include <IO/HTTPCommon.h> | ||
#include <IO/Progress.h> | ||
#include <IO/WriteBufferFromString.h> | ||
|
||
#include <IO/WriteHelpers.h> | ||
|
||
namespace DB | ||
{ | ||
|
@@ -29,28 +29,31 @@ void WriteBufferFromHTTPServerResponse::startSendHeaders() | |
} | ||
} | ||
|
||
void WriteBufferFromHTTPServerResponse::writeHeaderSummary() | ||
void WriteBufferFromHTTPServerResponse::writeHeaderProgressImpl(const char * header_name) | ||
{ | ||
if (headers_finished_sending) | ||
return; | ||
|
||
WriteBufferFromOwnString progress_string_writer; | ||
accumulated_progress.writeJSON(progress_string_writer); | ||
|
||
writeCString("{", progress_string_writer); | ||
accumulated_progress.writeJSON(progress_string_writer, false); | ||
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. This looks like trash. |
||
writeCString(",\"peak_memory_usage\":\"", progress_string_writer); | ||
writeText(peak_memory_usage, progress_string_writer); | ||
writeCString("\"}", progress_string_writer); | ||
|
||
if (response_header_ostr) | ||
*response_header_ostr << "X-ClickHouse-Summary: " << progress_string_writer.str() << "\r\n" << std::flush; | ||
*response_header_ostr << header_name << progress_string_writer.str() << "\r\n" << std::flush; | ||
} | ||
|
||
void WriteBufferFromHTTPServerResponse::writeHeaderProgress() | ||
void WriteBufferFromHTTPServerResponse::writeHeaderSummary() | ||
{ | ||
if (headers_finished_sending) | ||
return; | ||
|
||
WriteBufferFromOwnString progress_string_writer; | ||
accumulated_progress.writeJSON(progress_string_writer); | ||
writeHeaderProgressImpl("X-ClickHouse-Summary: "); | ||
} | ||
|
||
if (response_header_ostr) | ||
*response_header_ostr << "X-ClickHouse-Progress: " << progress_string_writer.str() << "\r\n" << std::flush; | ||
void WriteBufferFromHTTPServerResponse::writeHeaderProgress() | ||
{ | ||
writeHeaderProgressImpl("X-ClickHouse-Progress: "); | ||
} | ||
|
||
void WriteBufferFromHTTPServerResponse::writeExceptionCode() | ||
|
@@ -149,7 +152,7 @@ WriteBufferFromHTTPServerResponse::WriteBufferFromHTTPServerResponse( | |
} | ||
|
||
|
||
void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) | ||
void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress, Int64 peak_memory_usage_) | ||
{ | ||
std::lock_guard lock(mutex); | ||
|
||
|
@@ -158,7 +161,7 @@ void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) | |
return; | ||
|
||
accumulated_progress.incrementPiecewiseAtomically(progress); | ||
|
||
peak_memory_usage = peak_memory_usage_; | ||
if (send_progress && progress_watch.elapsed() >= send_progress_interval_ms * 1000000) | ||
{ | ||
progress_watch.restart(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,7 @@ class WriteBufferFromHTTPServerResponse final : public BufferWithOwnMemory<Write | |
~WriteBufferFromHTTPServerResponse() override; | ||
|
||
/// Writes progress in repeating HTTP headers. | ||
void onProgress(const Progress & progress); | ||
void onProgress(const Progress & progress, Int64 peak_memory_usage_); | ||
|
||
/// Turn compression on or off. | ||
/// The setting has any effect only if HTTP headers haven't been sent yet. | ||
|
@@ -89,6 +89,8 @@ class WriteBufferFromHTTPServerResponse final : public BufferWithOwnMemory<Write | |
/// but not finish them with \r\n, allowing to send more headers subsequently. | ||
void startSendHeaders(); | ||
|
||
// Used for write the header X-ClickHouse-Progress / X-ClickHouse-Summary | ||
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. Used for write -> Used for writing 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. Double whitespace. |
||
void writeHeaderProgressImpl(const char * header_name); | ||
// Used for write the header X-ClickHouse-Progress | ||
void writeHeaderProgress(); | ||
// Used for write the header X-ClickHouse-Summary | ||
|
@@ -126,6 +128,8 @@ class WriteBufferFromHTTPServerResponse final : public BufferWithOwnMemory<Write | |
|
||
int exception_code = 0; | ||
|
||
Int64 peak_memory_usage = 0; | ||
|
||
std::mutex mutex; /// progress callback could be called from different threads. | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -818,7 +818,11 @@ void HTTPHandler::processQuery( | |
|
||
/// While still no data has been sent, we will report about query execution progress by sending HTTP headers. | ||
/// Note that we add it unconditionally so the progress is available for `X-ClickHouse-Summary` | ||
append_callback([&used_output](const Progress & progress) { used_output.out->onProgress(progress); }); | ||
append_callback([&used_output](const Progress & progress) | ||
{ | ||
const auto& thread_group = CurrentThread::getGroup(); | ||
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. Wrong style: missing whitespace. |
||
used_output.out->onProgress(progress, thread_group->memory_tracker.getPeak()); | ||
}); | ||
|
||
if (settings.readonly > 0 && settings.cancel_http_readonly_queries_on_client_close) | ||
{ | ||
|
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.
I have one concern here how a new client works with an old server. Client would print
0
aspeak_memory_usage
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 seems to me that this entire line is for information, and it is unlikely that the client will rely on this value.
Try not to display the value for old servers?
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 would right and heavy. May be just enough not to log it when it zero. Just because memory usage never is zero.
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.
Added check (not show if old server). I use '-1' (as default) because memory can be zero (I don't know why it works like this; on small queries always 0).