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.

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

feat: add bytes_sent and bytes_received as metrics #624

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

rhatgadkar-goog
Copy link
Contributor

@rhatgadkar-goog rhatgadkar-goog commented Sep 19, 2024

@rhatgadkar-goog rhatgadkar-goog requested a review from a team as a code owner September 19, 2024 17:37
@rhatgadkar-goog rhatgadkar-goog linked an issue Sep 19, 2024 that may be closed by this pull request
Copy link
Collaborator

@nancynh nancynh left a comment

Choose a reason for hiding this comment

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

Did you check that the metrics were populated in monitoring? I'm not sure if you talked about this with Eno if that'd be too much of a hassle when OpenCensus is going away

Name: "alloydbconn/bytes_sent",
Measure: mBytesSent,
Description: "The number of bytes sent to an AlloyDB instance",
Aggregation: view.Sum(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be getting the LastValue instead of the Sum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the CloudSQL Go connector's code, and it used LastValue. But I'm thinking that we should use Sum instead, because we should be recording the total number of bytes sent/received.

With LastValue, the bytes_sent and bytes_received metrics would only show the most recent number of bytes that were sent/received. Not the accumulated sum. But I need to verify this in Cloud Monitoring.

Copy link
Member

Choose a reason for hiding this comment

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

cc @jackwotherspoon I can't remember if we talked about this or not. Sum is probably right, but I might have missed this in your PR review.

Copy link
Member

Choose a reason for hiding this comment

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

We also have count -- I forget what the difference between count and sum is here, but either way it should be a cumulative metric.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes LastValue was probably an oversight. I wonder whether Count or Sum is better...

I'm leaning towards count that way the monitoring platform (Cloud Monitoring) can decide to sum if it wants or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use Sum for now then if its the most simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this using both Sum() and Count(). I had a client program that does 2 operations: inserts a row into a table and then reads rows from the table. So as this program gets run more times, the number of bytes sent and received should increase over time.

I began using Sum() for both bytes_sent and bytes_received. And then at different times, I changed between using Count() and Sum(). It looked like this in Cloud Monitoring (this graph is for bytes_sent, but bytes_received had a similar shaped graph too):
cloudmonitoring

I'm thinking that Sum() looks more accurate, because the number of bytes sent is increasing over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, I'm thinking that as this program gets run more times, the number of bytes sent will be the same each time that it's run. But the number of bytes received will increase, because the table will grow larger.

The unaggregated graph in Cloud Monitoring might do a better job of showing this. I'll try testing this again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With Sum(), I verified that bytes_sent is constant:
sum_bytes_sent. And bytes_received increases over time:
sum_bytes_received

Count() has a similar graph, but was showing bytes_sent and bytes_received as less than 1/s. So I'm guessing this metric is measuring number of requests per unit of time. And not number of bytes per unit time. Here is an example of its bytes_received graph:
count_bytes_received

LastValue() has units of bytes. Not bytes/sec, unlike Sum(). But I think it's only showing the last number of bytes received from a read query, because it was only showing ~6k bytes received in this example:
last_value_bytes_received
In the graph above for bytes received for Sum(), it was showing ~68k bytes received over a 3 minute period.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you very much for testing all of this!

@rhatgadkar-goog
Copy link
Contributor Author

rhatgadkar-goog commented Sep 19, 2024

Did you check that the metrics were populated in monitoring? I'm not sure if you talked about this with Eno if that'd be too much of a hassle when OpenCensus is going away

Yes, I'm planning to to do this. I discussed with Eno that I'll use the Go connector in a client VM, send read/write requests to an AlloyDB cluster, and verify whether the metrics show up in Cloud Monitoring.

@nancynh
Copy link
Collaborator

nancynh commented Sep 19, 2024

Did you check that the metrics were populated in monitoring? I'm not sure if you talked about this with Eno if that'd be too much of a hassle when OpenCensus is going away

Yes, I'm planning to to do this. I discussed with Eno that I'll use the Go connector in a client VM, send read/write requests to an AlloyDB cluster, and verify whether the metrics show up in Cloud Monitoring.

Thanks! Do leave a comment on the PR when you verify this (and maybe some pictures would be nice :))

Copy link
Collaborator

@nancynh nancynh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rhatgadkar-goog rhatgadkar-goog merged commit 4aa27a5 into main Sep 27, 2024
16 checks passed
@rhatgadkar-goog rhatgadkar-goog deleted the bytes-metrics branch September 27, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port bytes_sent and bytes_received in metrics
4 participants