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.

Sign up for GitHub

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

Jump to bottom

insights bug: transaction overview page duplicate insights #91914

Closed
j82w opened this issue Nov 15, 2022 · 7 comments · Fixed by #91955
Closed

insights bug: transaction overview page duplicate insights #91914

j82w opened this issue Nov 15, 2022 · 7 comments · Fixed by #91955
Assignees
@xinhaoz
Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@j82w
Copy link
Contributor

j82w commented Nov 15, 2022 •
edited by cockroach-jira-scripts
Loading

This was reproduced with a clean branch. The insight "high contention" appears twice when it should only be there once.

Screen Shot 2022-11-15 at 10 33 52 AM

Jira issue: CRDB-21483
@j82w j82w added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-observability Related to observability of the SQL layer T-sql-observability labels Nov 15, 2022
@j82w
Copy link
Contributor Author

j82w commented Nov 15, 2022

Insights transaction details page shows N/A for all the top cards.
Screen Shot 2022-11-15 at 12 56 14 PM

@xinhaoz xinhaoz self-assigned this Nov 15, 2022
@xinhaoz
Copy link
Member

xinhaoz commented Nov 15, 2022

@j82w That's expected right now since there are no execution details available for txns with high contention as their insight. One option is to just not show these fields when they're not available.

@j82w
Copy link
Contributor Author

j82w commented Nov 15, 2022

@xinhaoz should it is say no sample like other pages when the info is not available? Is it not possible to calculate by aggregating the insight stmt information?

@xinhaoz
Copy link
Member

xinhaoz commented Nov 15, 2022 •
edited
Loading

@j82w no sample would also work here. We can't aggregate from the statements because there's no guarantee we record insights for executions that appear in the contention table. From my own testing I haven't seen it recorded, as the only time it would appear is if a single stmt in the txn had an execution of over 100ms, but for contention we take the total contention time as the number to compare with the threshold. Since the 2 dsata sources aren't very in sync with which transaction executions they have, for now we just opt to show the contention insight if we have results from both tables for a txn where the execution ids are different but fingerprints are the same.

xinhaoz added a commit to xinhaoz/cockroach that referenced this issue Nov 15, 2022
@xinhaoz
cluster-ui: transaction insights bug fixes
9296682 
Fixes cockroachdb#91914
Fixes cockroachdb#91915
Fixes cockroachdb#91927

Fixes for txn insights:
- De-duplicate txn contention insights labels in overview page for
high contention
- Remove query column for statement details and single stmt
transactions
- Include statement query for a txn insight recommendation if
there are multiple stmts in the txn being reported

Release note (ui change): Query column in insight recs table removed.
Instead, stmt is included in the description if the txn being reported
has multiple stmts.
@xinhaoz xinhaoz mentioned this issue Nov 15, 2022
Merged
@j82w
Copy link
Contributor Author

j82w commented Nov 16, 2022

@maryliag should it say 'N/A' when there is no info or 'no sample' in the top summary card?

@maryliag
Copy link
Contributor

if the reason is because we don't have the value (because we didn't collected on trace for example) use no sample, if is because the case wouldn't have no matter what, than you can use N/A

@xinhaoz
Copy link
Member

xinhaoz commented Nov 16, 2022

Sounds good, I'll update to no samples in my bug fix PR

craig bot pushed a commit that referenced this issue Nov 20, 2022
@xinhaoz
Merge #91955
84bea85 
91955: cluster-ui: transaction insights bug fixes r=xinhaoz a=xinhaoz

Fixes #91914
Fixes #91915
Fixes #91927

This commit fixes txn insights bugs and adds unit
tests for utils used by the insights pages.

Fixes for txn insights:
- De-duplicate txn contention insights labels in overview page for high contention
- Remove query column for statement details and single stmt transactions
- Include statement query for a txn insight recommendation if there are multiple stmts in the txn being reported

Release note (ui change): Query column in insight recs table removed. Instead, stmt is included in the description if the txn being reported has multiple stmts.

This txn has multiple stmts:
<img width="1678" alt="image" src="https://user-images.githubusercontent.com/20136951/202526318-601587ea-8b2c-4708-b058-b24cad66c71d.png">

Ths txn has 1 stmt:
<img width="1736" alt="image" src="https://user-images.githubusercontent.com/20136951/202040846-f02faecf-0c68-4b2c-bca3-ac813ac94a3a.png">

No change to stmts (but query col is removed everywhere):
<img width="1734" alt="image" src="https://user-images.githubusercontent.com/20136951/202040904-d21c1ed3-ed91-4cf7-b77e-bc74736f7d44.png">


Co-authored-by: Xin Hao Zhang <[email protected]>
@craig craig bot closed this as completed in 3177bc5 Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xinhaoz xinhaoz

Labels
A-sql-observability Related to observability of the SQL layer C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cluster-ui: transaction insights bug fixes xinhaoz/cockroach
3 participants
@maryliag @j82w @xinhaoz