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

ui: link to waiting, blocking txn details from waiting txn details #87876

Conversation

ericharmeling
Copy link
Contributor

This commit adds a link to the transaction insight details page for a blocking transaction, if that transaction is also waiting on another transaction.

Fixes #87784.

Release note: None
Release justification: high-benefit change to existing functionality

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling ericharmeling force-pushed the txn-details-fingerprint-link branch from 4ab09c8 to c01db7e Compare September 16, 2022 17:41
This commit adds a link to the transaction insight details page for
a blocking transaction, if that transaction is also waiting on another
transaction.

Fixes cockroachdb#87784.

Release note: None
Release justification: high-benefit change to existing functionality
@ericharmeling ericharmeling force-pushed the txn-details-fingerprint-link branch from c01db7e to cb9bef9 Compare September 16, 2022 18:14
@ericharmeling
Copy link
Contributor Author

ericharmeling commented Sep 20, 2022

When locally testing the changes in this PR, I came across another issue: blocking transactions are not also recorded as waiting transactions (see #88240).

I'm not sure if the behavior described in is expected/intentional behavior. If it is, then we might need to close this PR and the issue that it fixes.

@ericharmeling ericharmeling marked this pull request as ready for review September 20, 2022 15:22
@ericharmeling ericharmeling requested a review from a team September 23, 2022 00:46
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 all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)


-- commits line 5 at r1:
I don't think this is what the issue means. I think @kevin-v-ngo was saying that we should add the link to that execution id, because when going to the details of that one we could see more info about, such as other executions waiting on that. I don't think it means you should add the link only when it has other waiting for it.
Also, can you add image/video of the changes?

@kevin-v-ngo
Copy link

That's right and more importantly, see

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)

-- commits line 5 at r1: I don't think this is what the issue means. I think @kevin-v-ngo was saying that we should add the link to that execution id, because when going to the details of that one we could see more info about, such as other executions waiting on that. I don't think it means you should add the link only when it has other waiting for it. Also, can you add image/video of the changes?

That's right. And more importantly, see other transactions that blocked it as well. Basically navigating through the lock chain. And yeah, if there weren't any transactions that were blocked or waited for that linked transaction, i dont see a reason why we should not allow the user to drill into the transaction. Unless that information is not there? It should be since it blocked the current transaction though so there should be an event.

@ericharmeling
Copy link
Contributor Author

I think @kevin-v-ngo was saying that we should add the link to that execution id, because when going to the details of that one we could see more info about, such as other executions waiting on that. I don't think it means you should add the link only when it has other waiting for it.

I'm pretty sure transaction_contention_events only records transactions that have finished executing, so there won't be any active execution IDs available to link to an active execution ID details page. I suppose we could link to the transaction fingerprint details, but that page wouldn't include any information about specific blocking transactions, since it's all aggregated stats.

@ericharmeling
Copy link
Contributor Author

ericharmeling commented Nov 29, 2022

Closing this PR (see my comment above). I think resolving #87750 and #83780 and #91696 should be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants