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

server: stub out interface for transaction stats #52251

Closed

Conversation

solongordon
Copy link
Contributor

To support adding transaction-level statistics to the webui, I modified
the existing StatementsResponse to include a new transactions field.
Transactions here are defined by the collection of statement
fingerprints within. To allow referencing statements, I added a
statement ID field, which is a hash of the statement fingerprint.

For the moment, transactions in the response are faked out by combining
together statement IDs at random. This is meant to unblock development
on the frontend while we work on populating the response with the real
transaction data.

Release note: None

To support adding transaction-level statistics to the webui, I modified
the existing StatementsResponse to include a new `transactions` field.
Transactions here are defined by the collection of statement
fingerprints within. To allow referencing statements, I added a
statement ID field, which is a hash of the statement fingerprint.

For the moment, transactions in the response are faked out by combining
together statement IDs at random. This is meant to unblock development
on the frontend while we work on populating the response with the real
transaction data.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor Author

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @dhartunian)


pkg/roachpb/app_stats.proto, line 152 at r1 (raw file):

  // Id is a hash of the query fingerprint which can be used to identify the
  // statement, for instance in transaction statistics.
  optional string id = 8 [(gogoproto.nullable) = false];

@dhartunian Is it easy enough to use this ID to create an id -> statement map on the frontend side? Or is it worth monkeying with the response format so that the statement statistics would already be in that format?

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

Some minor nits, the only question I have is how this new proto interacts with TxnStats, which is used by appStats currently. Maybe we should collapse both these structures into 1?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @dhartunian, and @solongordon)


pkg/roachpb/app_stats.proto, line 92 at r1 (raw file):

}

message TransactionStatistics {

Can/should we unify this with TxnStats?


pkg/roachpb/app_stats.proto, line 98 at r1 (raw file):

  // Count is the total number of times this transaction was executed
  // since the begin of the reporting period.

nit: s/begin/beginning/


pkg/roachpb/app_stats.proto, line 109 at r1 (raw file):

  optional NumericStat num_rows = 4 [(gogoproto.nullable) = false];

  // ServiceLat is the time to service the transaction, from start of parse to

nit: it might be nice to re-word this comment as "time to service the transaction, from the time a transaction was received to end of execution".

I'm not sure if there could be a delay between a query being received and parsing starting, but we calculate service latency using TimeReceived.


pkg/server/statements.go, line 105 at r1 (raw file):

		for _, stmt := range response.Statements[i : i+numStatements] {
			txnStats.StatementIds = append(txnStats.StatementIds, stmt.Key.KeyData.Id)
			txnStats.Count++

Can we stub this out to something random instead of the number of statements in the transaction? I got confused for a second by what Count was referring to until I saw the comment on the proto.


pkg/server/serverpb/status.proto, line 805 at r1 (raw file):

}

message StatementsResponse {

Should we update this name? I don't like the idea of a StatementResponse encompassing both statement and transaction statistics.

@solongordon
Copy link
Contributor Author

Incorporated into #52704

@solongordon solongordon deleted the transaction-statistics branch August 24, 2020 14:01
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.

3 participants