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

Refactor and Query Discovery Mode #21

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Conversation

altmannmarcelo
Copy link
Collaborator

  • Refactor the code to make it more readable and maintainable. Added
    a new module proxysql to handle all the proxysql related
    functionalities.

  • Refactor the code to use the query module to handle all the
    query discovery related functionalities.

  • Added Query Discovery configuration. Now users can specify the
    query discovery mode in the configuration file. The set of rules
    to discover the queries are defined in the README.md file.

This commit adds the ability to read a list of hosts from mysql_servers
table instead of a single host from config file.

This merges the old server.rs into the hosts file.
This also remove the Pooled connection to a single connection.

Fixes: #5
- Refactor the code to make it more readable and maintainable. Added
    a new module `proxysql` to handle all the proxysql related
    functionalities.
- Refactor the code to use the `query` module to handle all the
    query discovery related functionalities.

- Added Query Discovery configuration. Now users can specify the
    query discovery mode in the configuration file. The set of rules
    to discover the queries are defined in the README.md file.

Closes: #6
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/hosts.rs Outdated Show resolved Hide resolved
src/proxysql.rs Show resolved Hide resolved
src/proxysql.rs Outdated Show resolved Hide resolved
src/proxysql.rs Show resolved Hide resolved
/// A boolean indicating if the rule was added successfully.
pub fn add_as_query_rule(&mut self, query: &Query) -> Result<bool, mysql::Error> {
let datetime_now: DateTime<Local> = Local::now();
let date_formatted = datetime_now.format("%Y-%m-%d %H:%M:%S");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to write the time zone here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to complicate here. This will always be run on the same ProxySQL server, so no timezone diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm only bringing this up because I see we're decoding some time zone below with %z. That seems okay?

.trim();
let datetime_mirror_str = format!("{} {}", datetime_mirror_str, tz);
let datetime_mirror_rule =
DateTime::parse_from_str(datetime_mirror_str.as_str(), "%Y-%m-%d %H:%M:%S %z")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the time zone decoding line I'm talking about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, ok - parse_from_str requires a TZ and we use localTZ.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. I missed the part where we append the time zone to the string after we fetch it from ProxySQL.

/// A boolean indicating if the rule was added successfully.
pub fn add_as_query_rule(&mut self, query: &Query) -> Result<bool, mysql::Error> {
let datetime_now: DateTime<Local> = Local::now();
let date_formatted = datetime_now.format("%Y-%m-%d %H:%M:%S");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm only bringing this up because I see we're decoding some time zone below with %z. That seems okay?

Fixed review comments in hosts and proxysql.cnf files.
Added measure unit to README.md to clarify the values are measured by
proxysql using microseconds.
Copy link
Contributor

@davisjc davisjc left a comment

Choose a reason for hiding this comment

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

Thanks!

@altmannmarcelo altmannmarcelo merged commit 5874962 into main Sep 10, 2024
4 checks passed
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.

2 participants