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 Prometheus metric to track sql stmts per transaction #29

Conversation

yvardhineni
Copy link
Contributor

@yvardhineni yvardhineni commented Jun 13, 2024

  • Added a new summary metric transaction_sql_stmts to track the number of exec/querys stmts executed per transaction
  • Introduced a new struct managedTx to to hold the context and connection of a transaction
  • Introduced a new field execStmtsCounter and queryStmtsCounter in the managedConn to track the number of exec querys in a transaction.
  • When SQL lib opens a transaction we are storing the connection for that transaction and context of the transaction in the managedTx.
  • Context is used to store the required grpc labels for the metric. i.e. grpc method, grpc service
  • Added a UnaryInterceptor to the grpc server to store the grpc method and grpc service in the context
  • On commit or rollback of the transaction we are removing the connection from the managedTx and resetting the counters to 0
  • On commit of the transaction we are observing the transaction_sql_stmts metric by the number of exec/querys got exexuted in the transaction

* Added a new summary metric `transaction_exec_query_total` to track the
  number of exec querys per transaction
* Introduced a new struct `managedTx` to to hold the context and
  connection of a transaction
* Introduced a new field `execQueryCounter` in the `managedConn` to
  track the number of exec querys in a transaction.
* When SQL lib opens a transaction we are storing the connection for
  that transacation and context of the transaction in the `managedTx`.
* Context is used to store the required grpc labels for the metric. i.e.
  grpc method, grpc service
* Added a UnaryInterceptor to the grpc server to store the grpc method
  and grpc service in the context
* On commit or rollback of the transaction we are removing the connection
  from the `managedTx` and resetting the execQueryCounter to 0
* On commit of the transaction we are observing the
  `transaction_exec_query_total` metric by the number of exec querys in
  the transaction
@yvardhineni
Copy link
Contributor Author

sample metrics:

# HELP transaction_exec_query_total The number of times exec query is called in a transaction
# TYPE transaction_exec_query_total summary
transaction_exec_query_total_sum{grpc_method="SayHello",grpc_service="helloworld.Greeter"} 4
transaction_exec_query_total_count{grpc_method="SayHello",grpc_service="helloworld.Greeter"} 2

daniel-garcia
daniel-garcia previously approved these changes Jun 13, 2024
Copy link
Contributor

@daniel-garcia daniel-garcia left a comment

Choose a reason for hiding this comment

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

LGTM but add some tests for this.

conn.go Outdated
@@ -79,6 +86,7 @@ func (c *managedConn) ExecContext(ctx context.Context, query string, args []driv
if !ok {
return nil, driver.ErrSkip
}
c.incExecQueryCounter() //increment the exec counter to keep track of the number of exec calls
Copy link
Contributor

Choose a reason for hiding this comment

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

The Exec() call above also needs an inc()

* Added two counter to track exec and query statements seperately.
* UTs added for the same.
@yvardhineni yvardhineni force-pushed the add_metric_to_track_exec_querys_per_transaction branch from 8aceee8 to 7f60ff2 Compare June 18, 2024 18:01
@yvardhineni
Copy link
Contributor Author

Updated Metrics:

# HELP transaction_sql_stmts_total The number of sql stmts called in a transaction by statement type per grpc service and method
# TYPE transaction_sql_stmts_total summary
transaction_sql_stmts_total_sum{grpc_method="List",grpc_service="infoblox.service.SampleService",stmt="exec"} 4
transaction_sql_stmts_total_count{grpc_method="List",grpc_service="infoblox.service.SampleService",stmt="exec"} 2
transaction_sql_stmts_total_sum{grpc_method="List",grpc_service="infoblox.service.SampleService",stmt="query"} 2
transaction_sql_stmts_total_count{grpc_method="List",grpc_service="infoblox.service.SampleService",stmt="query"} 2

@yvardhineni yvardhineni changed the title feat: Add Prometheus metric to track exec querys per transaction feat: Add Prometheus metric to track sql stmts per transaction Jun 18, 2024
@yvardhineni yvardhineni requested a review from kd7lxl June 18, 2024 18:23
Copy link
Contributor

@kd7lxl kd7lxl left a comment

Choose a reason for hiding this comment

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

The naming convention is to suffix counters with _total. Since this is a summary, I think stmnts is the unit. I don't see any examples in the best practices of summaries named like counters. So naming might be like transaction_sql_stmts:

  • transaction_sql_stmts_sum
  • transaction_sql_stmts_count

This doesn't seem related to hotload. I would like to see it as a separate module with a signature like dbmetrics.WrapDB(*sql.DB) *sql.DB. (You can add other arguments and error if it would be useful.) We can still wrap hotload if we want, but it would also be useful on its own.

@yvardhineni yvardhineni requested a review from kd7lxl June 18, 2024 19:28
@daniel-garcia
Copy link
Contributor

The naming convention is to suffix counters with _total. Since this is a summary, I think stmnts is the unit. I don't see any examples in the best practices of summaries named like counters. So naming might be like transaction_sql_stmts:

  • transaction_sql_stmts_sum
  • transaction_sql_stmts_count

This doesn't seem related to hotload. I would like to see it as a separate module with a signature like dbmetrics.WrapDB(*sql.DB) *sql.DB. (You can add other arguments and error if it would be useful.) We can still wrap hotload if we want, but it would also be useful on its own.

It may be useful as a standalone module however, we need this to be enabled on sql.Open so that we minimize changes to app code. Not it all instances can apps wrap the created sql.DB object.

@yvardhineni
Copy link
Contributor Author

The naming convention is to suffix counters with _total. Since this is a summary, I think stmnts is the unit. I don't see any examples in the best practices of summaries named like counters. So naming might be like transaction_sql_stmts:

  • transaction_sql_stmts_sum
  • transaction_sql_stmts_count

Updated the name to transaction_sql_stmts

daniel-garcia
daniel-garcia previously approved these changes Jun 19, 2024
prometheus.go Outdated Show resolved Hide resolved
@yvardhineni yvardhineni merged commit 09c77f9 into infobloxopen:main Jun 19, 2024
2 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.

None yet

3 participants