Skip to content

system.metric_log table #6467

Merged
alexey-milovidov merged 23 commits intoClickHouse:masterfrom
nikitamikhaylov:system_metric_log
Aug 15, 2019
Merged

system.metric_log table #6467
alexey-milovidov merged 23 commits intoClickHouse:masterfrom
nikitamikhaylov:system_metric_log

Conversation

@nikitamikhaylov
Copy link
Copy Markdown
Member

@nikitamikhaylov nikitamikhaylov commented Aug 13, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category (leave one):

  • New Feature

Short description (up to few sentences):

Detailed description (optional):
To enable add

    <metric_log>
        <database>system</database>
        <table>metric_log</table>
        <flush_interval_milliseconds>7500</flush_interval_milliseconds>
        <collect_interval_milliseconds>1000</collect_interval_milliseconds>
    </metric_log>

to config.xml.

#6363

@nikitamikhaylov nikitamikhaylov added the do not test disable testing on pull request label Aug 13, 2019
@nikitamikhaylov nikitamikhaylov added can be tested and removed do not test disable testing on pull request labels Aug 13, 2019
std::string name;
name += "ProfileEvent_";
name += ProfileEvents::getName(ProfileEvents::Event(i));
columns_with_type_and_name.emplace_back(std::make_shared<DataTypeUInt64>(), name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::move... or better: to construct inplace.

//ProfileEvents
for (size_t i = 0, end = ProfileEvents::end(); i < end; ++i)
{
const UInt64 value = ProfileEvents::global_counters[i];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.load(std::memory_order_relaxed)


if (metric_log)
{
constexpr size_t DEFAULT_METRIC_LOG_COLLECT_INTERVAL_MILLISECONDS = 1000;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's make no default at all. (config.get... with throw an exception) and keep the value in example in config.

elem.event_time = std::chrono::system_clock::to_time_t(prev_timepoint);
this->add(elem);

desired_timepoint = prev_timepoint + std::chrono::milliseconds(collect_interval_milliseconds);
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov Aug 14, 2019

Choose a reason for hiding this comment

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

What if you kill -STOP clickhouse-server* for a minute and then continue?
In you implementation, it will write multiple identical values after wakeup.

But it's better to skip dropped time points.

* it's totally real on production servers due to IO overload and page faults.

@alexey-milovidov
Copy link
Copy Markdown
Member

Let's merge and wait for docker image to rebuild.

@alexey-milovidov alexey-milovidov merged commit a70bfa1 into ClickHouse:master Aug 15, 2019
@nikitamikhaylov nikitamikhaylov added the pr-feature Pull request with new product feature label Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-feature Pull request with new product feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants