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, sql: collect additional session details #80717

Closed
wants to merge 1 commit into from

Conversation

gtr
Copy link
Contributor

@gtr gtr commented Apr 28, 2022

Partially addresses #74257.

Previously, the sql server did not provide additional session details
such as total number of transactions executed, transaction fingerprint
IDs, and total active time. This change adds the aforementioned session
details to the serverpb.Session struct.

To track transaction fingerprint IDs, a new circular buffer
TxnFingerprintIDBuffer is introduced with its corresponding cluster
setting TxnFingerprintIDBufferCapacity which controls the capacity.
The total number of transactions executed is accumulated using the txnCounter
from the extraTxnState in connExecutor. The total active time is calculated
by a timeutil.StopWatch which is started and stopped when a transaction is
started, restarted, and finished.

Release note (api change): the serverpb.Session struct now has three
new fields: number of transactions executed, transaction fingerprint
IDs, and total active time.

@gtr gtr requested review from a team as code owners April 28, 2022 17:01
@gtr gtr requested a review from a team April 28, 2022 17:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@gtr gtr removed request for a team April 28, 2022 17:02
@gtr gtr marked this pull request as draft April 28, 2022 17:02
@gtr gtr force-pushed the gtr-session-details branch 2 times, most recently from cd3fb6f to 79d16c0 Compare April 28, 2022 20:44
@gtr gtr marked this pull request as ready for review May 2, 2022 16:18
@maryliag maryliag requested a review from a team May 2, 2022 16:58
@gtr gtr force-pushed the gtr-session-details branch from 79d16c0 to 46dce48 Compare May 2, 2022 18:52
@blathers-crl
Copy link

blathers-crl bot commented May 2, 2022

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label May 2, 2022
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 11 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr and @maryliag)


docs/generated/swagger/spec.json line 1230 at r2 (raw file):

        },
        "num_txns_executed": {
          "description": "num_txns_executed is the number of transactions that were executed so\nfar on this session.",

nit: start directly with Number of transactions...


docs/generated/swagger/spec.json line 1520 at r2 (raw file):

    },
    "TransactionFingerprintID": {
      "description": "TransactionFingerprintID is the hashed string constructed using the\nindividual statement fingerprint IDs that comprise the transaction.",

nit: start with Hashed string...

@gtr gtr force-pushed the gtr-session-details branch from 46dce48 to a6cc1c4 Compare May 3, 2022 14:52
Copy link
Contributor Author

@gtr gtr 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 @maryliag)


docs/generated/swagger/spec.json line 1230 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: start directly with Number of transactions...

Done.


docs/generated/swagger/spec.json line 1520 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: start with Hashed string...

I think this description comes from the comment on roachpb/app_stats.go:
Should I still change it?

Code snippet:

// TransactionFingerprintID is the hashed string constructed using the
// individual statement fingerprint IDs that comprise the transaction.
type TransactionFingerprintID uint64

Copy link
Contributor

@Azhng Azhng 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 @Azhng, @gtr, and @maryliag)


pkg/sql/conn_executor.go line 3135 at r3 (raw file):

		return nil
	}
	err := ex.applicationStats.IterateTransactionStats(context.Background(), options, txnStatsVisitor)

Not sure using applicationStats is a good idea here, applicationStats can be shared by multiple connExecutors concurrently, (in the case of explicit transaction, a separate instance of the applicationStats is used exclusively for that transaction).

SQL Stats in general wasn't designed with tracking session-level information in mind, and it's quite difficult with how the fingerprinting is implement. (it doesn't quite make sense to have a "session fingerprint"). If you really want to have a session level fingerprints, you might have to implement it from scratch.

@gtr gtr force-pushed the gtr-session-details branch from a6cc1c4 to d9304ce Compare May 4, 2022 19:05
Copy link
Contributor Author

@gtr gtr 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 @Azhng and @maryliag)


pkg/sql/conn_executor.go line 3135 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Not sure using applicationStats is a good idea here, applicationStats can be shared by multiple connExecutors concurrently, (in the case of explicit transaction, a separate instance of the applicationStats is used exclusively for that transaction).

SQL Stats in general wasn't designed with tracking session-level information in mind, and it's quite difficult with how the fingerprinting is implement. (it doesn't quite make sense to have a "session fingerprint"). If you really want to have a session level fingerprints, you might have to implement it from scratch.

Good catch, I had a feeling the applicationStats were not unique to a ConnExecutor. I ended up going with a new TxnFingerprintIDBuffer which is a circular buffer tracking the last 100 txnFingerprintIDs like we discussed.

@gtr gtr force-pushed the gtr-session-details branch 2 times, most recently from 6442c2e to 06647aa Compare May 5, 2022 15:26
Partially addresses cockroachdb#74257.

Previously, the sql server did not provide additional session details
such as total number of transactions executed, transaction fingerprint
IDs, and total active time. This change adds the aforementioned session
details to the `serverpb.Session` struct.

To track transaction fingerprint IDs, a new circular buffer
`TxnFingerprintIDBuffer` is introduced with its corresponding cluster
setting `TxnFingerprintIDBufferCapacity` which controls the capacity.
The total number of transactions executed is accumulated using the `txnCounter`
from the `extraTxnState` in `connExecutor`. The total active time is calculated
by a `timeutil.StopWatch` which is started and stopped when a transaction is
started, restarted, and finished.

Release note (api change): the `serverpb.Session` struct now has three
new fields: number of transactions executed, transaction fingerprint
IDs, and total active time.
@gtr gtr force-pushed the gtr-session-details branch from 06647aa to aa65054 Compare May 6, 2022 19:14
Copy link
Contributor

@Azhng Azhng 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 @Azhng, @gtr, and @maryliag)


pkg/sql/txn_fingerprint_id_buffer.go line 34 at r6 (raw file):

// TxnFingerprintIDBuffer is a thread-safe circular buffer tracking transaction
// fingerprint IDs at the session level.
type TxnFingerprintIDBuffer struct {

nit: do we need to export this struct ?


pkg/sql/txn_fingerprint_id_buffer.go line 70 at r6 (raw file):

	b.mu.data = make([]roachpb.TransactionFingerprintID, capacity)
	for i := range b.mu.data {
		b.mu.data[i] = roachpb.InvalidTransactionFingerprintID

nit: roachpb.InvalidTransactionFingerprintID is the same as the default initialization value of the type. This initialization loop doesn't seem to be necessary.


pkg/sql/txn_fingerprint_id_buffer.go line 105 at r6 (raw file):

func (b *TxnFingerprintIDBuffer) checkForCapacityLocked() {
	capacityClusterSetting := TxnFingerprintIDBufferCapacity.Get(&b.st.SV)
	if b.mu.capacity != capacityClusterSetting {

you can just use cap() builtin function to check capacity instead of track it separately


pkg/sql/txn_fingerprint_id_buffer.go line 118 at r6 (raw file):

	ptr := 0
	b.mu.size = 0
	b.initializeBufferLocked(newCapacity)

Doesn't initializeBufferLocked() directly overwrites the b.mu.data ? then this would make the newData allocation above unnecessary.

Later we then assign the newData (newly allocated) to the b.mu.data (also newly allocated), seems like we can just skip the allocation here.


pkg/sql/txn_fingerprint_id_buffer.go line 138 at r6 (raw file):

// circular buffer.
func (b *TxnFingerprintIDBuffer) GetAllTxnFingerprintIDs() []roachpb.TransactionFingerprintID {
	b.mu.Lock()

hold the read lock here instead of write lock?


pkg/sql/txn_fingerprint_id_buffer.go line 141 at r6 (raw file):

	defer b.mu.Unlock()

	var txnFingerprintIDs []roachpb.TransactionFingerprintID

we can initialize the entire buffer here since we already know the size of the buffer


pkg/sql/txn_fingerprint_id_buffer.go line 170 at r6 (raw file):

	size := txnFingerprintID.Size()
	b.mu.acc.Shrink(context.Background(), size)

Since the memory is allocated once and reused, I feel we can just update the memory account each time we need to reallocate the capacity.


pkg/sql/txn_fingerprint_id_buffer.go line 172 at r6 (raw file):

	b.mu.acc.Shrink(context.Background(), size)
	b.mu.size--
	b.mu.readPosition++

we should wrap the read position around if the index is out of bound no ?

craig bot pushed a commit that referenced this pull request Jun 28, 2022
82352: server, sql: surface session txnCount, recent txn fingerprints, active time r=xinhaoz a=xinhaoz

Finishing up Gerardo's PR, original review here: #80717

--------------------------------------------------
Partially addresses #74257.

Previously, the status server did not provide session details such as
total number of transactions executed, transaction fingerprint
IDs, and total active time. This change adds the aforementioned session
details to the `serverpb.Session` struct.

To track recently executed transaction fingerprint IDs, a FIFO cache
`TxnFingerprintIDCache` is introduced with its corresponding cluster
setting `TxnFingerprintIDBufferCapacity` to control the capacity. The
default capacity is set at 100 fingerprints.

The total number of transactions executed is filled using the existing
`txnCounter` from the `extraTxnState` in `connExecutor`. The total active
time is calculated by introducing a `timeutil.StopWatch` to the connection
executor, which is started and stopped when a transaction is started and
finished respectively.

Release note (api change): the `serverpb.Session` struct now has three
new fields: number of transactions executed, transaction fingerprint
IDs, and total active time.

82623: backupinfo: introduce a backupinfo package r=stevendanna a=adityamaru

The backupinfo package contains logic related to interacting
with information and metadata describing the backup. After this
change we have `backupdest` depending on `backupinfo`.

Release note: None

82718: kvserver: emit MVCC range tombstones over rangefeeds r=aliher1911 a=erikgrinaker

This patch adds MVCC range tombstone support in rangefeeds. Whenever an
MVCC range tombstone is written, a new `MVCCDeleteRangeOp` logical op
is recorded and emitted across the rangefeed as a `RangeFeedDeleteRange`
event. MVCC range tombstones will only be written when the
`MVCCRangeTombstones` version gate has been enabled.

Changefeeds will emit an error for these events. We do not expect to see
these in online spans with changefeeds, since they are initially only
planned for use with schema GC and import rollbacks.

The rangefeed client library has been extended with support for these
events, but no existing callers handle them for the same reason as
changefeeds. Initial scans do not emit regular tombstones, and thus not
range tombstones either, but catchup scans will emit them if
encountered.

This patch has rudimentary testing of MVCC range tombstones in
rangefeeds. A later patch will add a data-driven test harness for
rangefeeds with more exhaustive tests.

Resolves #82449.
Touches #70433.

Release note: None

82936: sql/schemachanger: implement DROP OWNED BY r=jasonmchan a=jasonmchan

Previously, we did not support the DROP OWNED BY statement (#55381).
This commit adds partial support for DROP OWNED BY in the declarative
schema changer. Followup work is needed to support the CASCADE modifier.

Release note (sql change): Support `DROP OWNED BY`.

83229: ui: remove option 10/30 min from SQL Activity page r=maryliag a=maryliag

Note to reviewers: only 2nd commit is relevant to this PR

Previously we had the options for 10 and 30min on
SQL Activity pages, which created some confusion, since
we would always show the last 1h info.
This commit remove those 2 options.
If the user select any of those options on the Metrics
page, it will get updated to 1h on the SQL Activity
pages.

<img width="444" alt="Screen Shot 2022-06-22 at 5 43 53 PM" src="https://user-images.githubusercontent.com/1017486/175144243-2f084e0b-5e09-4874-9640-e7eea6179343.png">

https://www.loom.com/share/226e54322df6456aa2039b5c54f72eb1


Fixes #82914

Release note (ui change): Removal of the 10 and 30min options
on the SQL Activity page.

83420: ui: improve tooltip UX with text updates r=ericharmeling a=ericharmeling

Fixes #81374.
Fixes #83256.
Fixes #81248.
Fixes #79018.

Note the following:

- The updates resolving #79018 effectively revert the tooltip text for Rows Read to the original wording (which [was updated for accuracy](e379e9d#diff-492398441e971e355a687a4ce333a9766e2195287d0227682444d5dc0eb7ee1a)). I assume this is okay. `@kevin-v-ngo`
- The updates resolving #81248 do not in fact refer to the time intervals as date ranges, as this language is misleading (a 1h interval is an interval and not a date range). Instead, this update just removes the anchor and the link to the non-existent Interval Range section of https://www.cockroachlabs.com/docs/stable/ui-statements-page.html. We may want to consider updating the docs to call the "time picker" data type a time interval and not a date range. This appears to have been the case in previous releases (https://www.cockroachlabs.com/docs/v21.1/ui-statements-page#time-interval). `@stbof` 

Release note (ui change): Updated tooltips on the Statements and
Transactions pages in the DB Console for improved UX.

83428: sql: rename anonymizedStmt in sqlstats pkg to stmtNoConstants r=ericharmeling a=ericharmeling

Note that this commit does not change any files outside the sqlstats package.

Fixes #80725.

Release note: None

83468: ui: update all dates to use same format r=maryliag a=maryliag

Update all dates to use the same format.

Fixes #81159

Release note: None

83520: kv: don't try to reject lease transfer when flushing proposal buffer r=nvanbenschoten a=nvanbenschoten

Fixes #83498.
Fixes #83402.
Fixes #83308.

This was fallout from #82758.

This commit adds logic to `propBuf.maybeRejectUnsafeProposalLocked` to avoid
trying to reject proposals based on the state of the raft group when the group
is not provided (e.g. when flushing the buffer). We already had this logic for
`RequestLease` (indirectly), but did not for `TransferLease`.

Co-authored-by: Gerardo Torres <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Jason Chan <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants