-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
builtins: add builtin to retrieve the payload(s) for a span. #60616
Conversation
bfdc0b9
to
18dd917
Compare
18dd917
to
6dcba7d
Compare
6dcba7d
to
f9f0faa
Compare
f9f0faa
to
18fbd2d
Compare
60512: backupccl: update backup detached tests with retry execution under transaction r=pbardea a=Elliebababa update backup and restore detached tests with retry execution under transaction Previously, backup and restore detached tests do not retry if transactions fail. They could be flaky tests and fail in CI check. With this PR, backup and restore detached tests under transaction will not be aborted by retry errors. Release notes: None. 60594: build: explicitly add unzip as a dependency r=rickystewart a=ulfjack The `unzip` package is not installed by default in Ubuntu. It's currently pulled in as an implicit dependency of the Bazel package, but that breaks if Bazel ever removes that dependency (or when changing the Dockerfile to not install Bazel). 60648: distsql: improve test harness for processors against operators r=yuzefovich a=yuzefovich Previously, when comparing the output of processors and operators, if any order is allowed, we would perform a quadratic search. This is suboptimal, and this commit improves the situation by sorting both outputs first and then using the ordered comparison. Additionally, this commit fixes a rare flake when seemingly the same outputs would result in a mismatch. I could somewhat reliably reproduce it before the patch and cannot reproduce it after the patch. It is likely to do something with the float comparison, but I didn't get to the bottom of the flake, yet I don't think it's worth it given that this commit improves the situation anyway. Fixes: #60608. Release note: None 60666: util/tracing: re-key activeSpans map on span ID r=irfansharif a=angelapwen Note that the changes in this PR should be used in #60616 to more easily retrieve a span given its span ID, without having to visit all spans and check for a match on span ID. Whichever PR lands later should update the sections of code marked as #TODOs in #60616. Previously, a tracer's activeSpans map was keyed on the memory address of the span itself. This commit keys the map on the span ID (which is deterministically unique) and sets the corresponding value in the map to be the memory address of the span itself. This allows us to continue to visit all active spans via the map, but also easily retrieve a span from its ID using the map. Release note: None 60679: licenses: Update CCL text r=petermattis a=bdarnell Remove a dead link and clarify related text Release note: None 60681: sql: fix spacing for comments around virtual schema r=irfansharif a=irfansharif Release note: None Co-authored-by: elliebababa <ellie24.huang@gmail.com> Co-authored-by: Ulf Adams <ulfjack@users.noreply.github.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: angelapwen <angelaw@cockroachlabs.com> Co-authored-by: Ben Darnell <ben@cockroachlabs.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Holding off on further review until we introduce a tabular for for span payloads)
I need to rebase on top of #60666 and then push up my newest changes that use the |
18fbd2d
to
640035e
Compare
Ok, have rebased and resolved comments 🙆♀️ if #59992 is merged soon I'm inclined to fix up the commits here and merge what we have, then work on improving the output of the built-in to return a table separately. Otherwise I'll also do that in this PR. |
I'd personally prefer to iterate right here, especially if we're considering changing the output form anyway. I imagine it'll end up undoing most of what was introduced in the last commit. |
@irfansharif ah, ignore the previous comment here — I'd thought you meant iterate over the spans but you meant iterate on the PR. 😸 That sounds fine to me. |
33d3166
to
b454338
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is OK as a transition point. Ok to merge this after you spell out what's going to happen next.
Another confirmation that the JSON array approach is defective is that it does not perform memory monitoring and so the accumulation in RAM can cause the node to crash with an OOM error.
This would be alleviated by a streaming approach, or adding memory monitoring to the current implementation).
Reviewed 15 of 15 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)
Thanks @knz ✨ To make sure I understand correctly, when I make the modification so that the built-in returns a table, there should not be an OOM error because the value generator functions stream values into rows default? As for what's going to happen next:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)
pkg/sql/sem/builtins/builtins.go, line 3611 at r11 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
Not anymore! Thanks, removing locally.
Done.
pkg/sql/sem/builtins/builtins.go, line 3625 at r11 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
Yep, you're right, this was a vestige of the string version we had played around with! Removing.
Done.
pkg/sql/sem/builtins/builtins.go, line 3629 at r11 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
You're right, though now that I am not going to use the
VisitSpans
method (because I can use theactiveSpans
map) this is no longer relevant.
Done.
pkg/sql/sem/builtins/builtins.go, line 3634 at r11 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
It doesn't ever evaluate to true — this is why I had never hit the panic when
NewDJSON
is passednil
yesterday. I'm removing this and in the next version where I get the span from its ID in the map, will return an empty JSON array if the span ID isn't found.
Done.
b454338
to
ffe8371
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a pretty column to the table returned and have the JSON pretty-printed as the value.
thanks, can you explain this point more?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @knz, and @tbg)
ffe8371
to
a83187b
Compare
@knz I meant that the built-in's table returned could also have a column called By the way, I was thinking — should the builtins for |
Yup. |
The `crdb_internal.payloads_for_span` builtin retrieves all payloads for a given span ID, given that the span is part of an active trace. The payloads are returned in JSONB format. If the span is not found, or if the span does not have any payloads, the builtin returns an empty JSON object. With the appropriate usage of this builtin and the `crdb_internal.trace_id` builtin as shown in the `contention_event` logic test, all payloads for the current trace may be surfaced. Release note (sql change): add `payloads_for_span` builtin that takes in a span ID and returns its paylods in JSONB format. If the span is not found, or if the span does not have any payloads, the builtin returns an empty JSON object.
a83187b
to
96b24ae
Compare
Added admin checks for the |
bors r=knz,irfansharif Thanks for the reviews 😄 |
This PR was included in a batch that was canceled, it will be automatically retried |
Build failed (retrying...): |
Build succeeded: |
60784: builtins: add generator builtin for span payloads r=irfansharif,knz a=angelapwen Following up from #60616. Part of addressing #55733. Previously we introduced a `crdb_internal.payloads_for_span` builtin that returned a JSONB array representing all payloads for a particular span. To improve the user experience of the builtin as well as prevent OOMs resulting from builtin usage, we replace it with a generator builtin that returns a table representing all payloads for a span, where each row represents a single payload. The new builtin, also called `crdb_internal.payloads_for_span`, has columns for the `payload_type` so that the user has the ability to easily filter on the type, and `payload_jsonb` so the user can use jsonb builtins to filter further. Release note (sql change): Update `crdb_internal.payloads_for_span` builtin to return a table instead of a JSONB array. Each row of the table represents one payload for the given span. It has columns for `payload_type` and `payload_jsonb`. Co-authored-by: angelapwen <angelaw@cockroachlabs.com>
Part of addressing #55733
The
payloads_for_span
builtin retrieves all payloads fora given span ID, given that the span is part of an active trace.
The payloads are returned in JSONB format. If the span is not
found, or if the span does not have any payloads, the builtin
returns an empty JSON object.
With the appropriate usage of this builtin and the
crdb_internal.trace_id
builtin as shown in thecontention_event
logic test, all payloads for the current trace may be surfaced.
Release note (sql change): add
payloads_for_span
builtin thattakes in a span ID and returns its paylods in JSONB format. If
the span is not found, or if the span does not have any payloads,
the builtin returns an empty JSON object.