Skip to content

NSFS | Adding metric type to the prefix + Removing the errors in the nsfs io count #7191

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

Merged
merged 2 commits into from
Feb 2, 2023
Merged

NSFS | Adding metric type to the prefix + Removing the errors in the nsfs io count #7191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bc3b00
Removing read and write error count for NSFS I/O metrics
liranmauda Feb 1, 2023
3021296
Adding to the nsfs metrics report the metric type (fs/op/io)
liranmauda Feb 1, 2023
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
23 changes: 17 additions & 6 deletions src/sdk/endpoint_stats_collector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class EndpointStatsCollector {
this.op_stats = {};
this.fs_workers_stats = {};
this.reset_all_stats();
this.nsfs_io_counters = this._new_namespace_stats();
this.nsfs_io_counters = this._new_namespace_nsfs_stats();
this.prom_metrics_report = prom_report.get_endpoint_report();
}

Expand All @@ -59,7 +59,7 @@ class EndpointStatsCollector {
}

reset_all_nsfs_stats() {
this.nsfs_io_counters = this._new_namespace_stats();
this.nsfs_io_counters = this._new_namespace_nsfs_stats();
this.op_stats = {};
this.fs_workers_stats = {};
}
Expand Down Expand Up @@ -115,6 +115,15 @@ class EndpointStatsCollector {
};
}

_new_namespace_nsfs_stats() {
return {
read_count: 0,
write_count: 0,
read_bytes: 0,
write_bytes: 0,
};
}

update_namespace_read_stats({ namespace_resource_id, bucket_name, size = 0, count = 0, is_err }) {
this.namespace_stats[namespace_resource_id] = this.namespace_stats[namespace_resource_id] || this._new_namespace_stats();
const io_stats = this.namespace_stats[namespace_resource_id];
Expand Down Expand Up @@ -218,8 +227,9 @@ class EndpointStatsCollector {

update_nsfs_read_counters({ size = 0, count = 0, is_err }) {
if (is_err) {
this.nsfs_io_counters.error_read_count += count;
this.nsfs_io_counters.error_read_bytes += size;
dbg.warn(`unexpectedly reached here upon error, we need to figure out why and maybe re-add the error counters`);
// this.nsfs_io_counters.error_read_count += count;
Copy link
Contributor

@shirady shirady Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that we should not leave code in comments in general.
If we would get here and need to debug it - we have version control.

If you insist on leaving them, at least I would suggest also leaving a comment here about why we added the code in a comment explicitly. For example:

Uncomment these lines in case we see the warning above in the logs.
I left them to help us debug it easily.
We can delete them if we will see that we don't have the warning above for a while.

@liranmauda, What do you think?

Copy link
Contributor Author

@liranmauda liranmauda Feb 2, 2023

Choose a reason for hiding this comment

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

The warning itself is the explanation.
here we will probably never reach the error path as it is in under layer, and the error will fail much earlier.
with that said, this code is relaying on an older code, and for the chance that we are missing a flow, we added the warning to explain and to see that we actually moved throw this flow.

once we get confidante we can remove the condition completely (the whole if, else will not be relevant).

// this.nsfs_io_counters.error_read_bytes += size;
} else {
this.nsfs_io_counters.read_count += count;
this.nsfs_io_counters.read_bytes += size;
Expand All @@ -233,8 +243,9 @@ class EndpointStatsCollector {

update_nsfs_write_counters({ size = 0, count = 0, is_err }) {
if (is_err) {
this.nsfs_io_counters.error_write_count += count;
this.nsfs_io_counters.error_write_bytes += size;
dbg.warn(`unexpectedly reached here upon error, we need to figure out why and maybe re-add the error counters`);
// this.nsfs_io_counters.error_write_count += count;
// this.nsfs_io_counters.error_write_bytes += size;
} else {
this.nsfs_io_counters.write_count += count;
this.nsfs_io_counters.write_bytes += size;
Expand Down
10 changes: 3 additions & 7 deletions src/server/system_services/stats_aggregator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ let last_partial_stats_requested = 0;
const PARTIAL_STATS_REQUESTED_GRACE_TIME = 30 * 1000;

// Will hold the nsfs counters/metrics
let nsfs_io_counters = _new_nsfs_stats();
let nsfs_io_counters = _new_namespace_nsfs_stats();
// Will hold the op stats (op name, min/max/avg time, count, error count)
let op_stats = {};
let fs_workers_stats = {};
Expand Down Expand Up @@ -1344,23 +1344,19 @@ function _set_fs_workers_stats(fsworker_name, stats) {
}
}

function _new_nsfs_stats() {
function _new_namespace_nsfs_stats() {
return {
read_count: 0,
write_count: 0,
read_bytes: 0,
write_bytes: 0,
error_write_bytes: 0,
error_write_count: 0,
error_read_bytes: 0,
error_read_count: 0,
};
}

// Will return the current nsfs_io_counters and reset it.
function get_nsfs_io_stats() {
const nsfs_io_stats = nsfs_io_counters;
nsfs_io_counters = _new_nsfs_stats();
nsfs_io_counters = _new_namespace_nsfs_stats();
return nsfs_io_stats;
}

Expand Down
6 changes: 3 additions & 3 deletions src/server/web_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ function _create_nsfs_report() {
const nsfs_counters = stats_aggregator.get_nsfs_io_stats();
// Building the report per io and value
for (const [key, value] of Object.entries(nsfs_counters)) {
const metric = `NooBaa_nsfs_${key}`.toLowerCase();
const metric = `noobaa_nsfs_io_${key}`.toLowerCase();
nsfs_report += `${metric}: ${value}<br>`;
}

Expand All @@ -309,7 +309,7 @@ function _create_nsfs_report() {
for (const [op_name, obj] of Object.entries(op_stats)) {
nsfs_report += `<br>`;
for (const [key, value] of Object.entries(obj)) {
const metric = `NooBaa_nsfs_${op_name}_${key}`.toLowerCase();
const metric = `noobaa_nsfs_op_${op_name}_${key}`.toLowerCase();
nsfs_report += `${metric}: ${value}<br>`;
}
}
Expand All @@ -319,7 +319,7 @@ function _create_nsfs_report() {
for (const [fs_worker_name, obj] of Object.entries(fs_workers_stats)) {
nsfs_report += `<br>`;
for (const [key, value] of Object.entries(obj)) {
const metric = `NooBaa_nsfs_${fs_worker_name}_${key}`.toLowerCase();
const metric = `noobaa_nsfs_fs_${fs_worker_name}_${key}`.toLowerCase();
nsfs_report += `${metric}: ${value}<br>`;
}
}
Expand Down