builtins: add generator builtin for span payloads #60784

angelapwen · 2021-02-19T09:52:45Z

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.

cockroach-teamcity · 2021-02-19T09:52:51Z

This change is

angelapwen · 2021-02-19T10:06:05Z

pkg/sql/logictest/testdata/logic_test/contention_event, line 68 at r2 (raw file):

# TODO(angelapwen) This should have 2 columns, payload_type and full_payload.
query T colnames
WITH spans AS (

This PR is not ready yet because I am trying to figure out why the builtin (named crdb_internal.payloads_for_span_as_table for now actually seems to return one column where each row is a tuple. It should return two columns, payload_type and full_payload so that we are able to filter on them.

I seem to be adhering to the way to return a row in the generator overload correctly, and in the functions.md doc everything seems to be labeled appropriately (see crdb_internal.check_consistency as an example of another generator builtin that returns several columns.

After I can get the builtin to return the columns, I will modify this logic test to look more like the one above (filtering on the payload_type column). This is the current output of the logic test so that reviewers can see the status of the builtin.

knz · 2021-02-19T11:16:22Z

for now actually seems to return one column where each row is a tuple. It should return two columns

It does return two columns.

Try this: SELECT * FROM crdb_internal.payloads_for_span_as_table(123)

angelapwen · 2021-02-19T11:20:08Z

@knz when I do so I only get one column, which is called crdb_internal.payloads_for_span_as_table:

query T colnames
SELECT crdb_internal.payloads_for_span_as_table(123);
 ----
crdb_internal.payloads_for_span_as_table

If I run it with query TT I get an "expected two columns, but found one" error.

knz · 2021-02-19T11:26:22Z

I wrote SELECT * FROM ..., not SELECT .... The two forms are different behavior.

angelapwen · 2021-02-19T11:32:11Z

A ha I missed that! I see. So it is an issue with the way my query is written and not in the generator.

knz · 2021-02-19T11:40:33Z

It's not really an "issue" - that's the regular, expected semantics of set-returning functions.

if you use a SRF as a table, then it returns a table
if you use a SRF as a scalar, then it returns a tuple

angelapwen · 2021-02-19T12:04:31Z

Understood, I meant it was an 'issue' with the way I wrote the logic test and expected a table after using the generator as a scalar. I'm having trouble with the syntax then — how can I generate a table comprised of sub-tables that each represent the return value of the SRF as a table for a span ID?

Basically I want to write something like

  SELECT span_id
  FROM crdb_internal.node_inflight_trace_spans
  WHERE trace_id = crdb_internal.trace_id()
) SELECT *
  FROM crdb_internal.payloads_for_span_as_table(SELECT * FROM spans);

but I cannot pass in a table as the arguments to the SRF.

knz · 2021-02-19T12:20:34Z

You need to use the LATERAL keyword as explained here https://www.postgresql.org/docs/13/queries-table-expressions.html

angelapwen · 2021-02-19T13:00:09Z

pkg/sql/logictest/testdata/logic_test/builtin_function, line 2921 at r3 (raw file):

query TTT colnames
SELECT *
FROM crdb_internal.payloads_for_span(0)

I wanted to write a logic test for the schema of the built-in but I'm not sure I can simply pass in 0 as the span — it is possible that a span's ID is 0 right?

angelapwen

Thank you @knz TIL about LATERAL!

The PR is now ready for review, and I added a couple of questions/comments of mine as well related to testing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @knz, and @tbg)

pkg/sql/logictest/testdata/logic_test/contention_event, line 62 at r3 (raw file):

) SELECT count(*) > 0
  FROM payloads
  WHERE payload_type = '"type.googleapis.com/cockroach.roachpb.ContentionEvent"';

I fetch the value of the type by using FetchValKey() in JSON which replicates the -> operator in the JSONB builtin. There is no replication for ->> which prints the value in string format (without the double quotes). Is this an alright user experience? I'm not sure a user would know to filter on a double-quoted in the string unless they SELECT * from the table first.

I can strip the double quotes in a hacky way, unless there is a better way.

irfansharif

Reviewed 1 of 5 files at r1, 3 of 5 files at r3, 1 of 4 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, @knz, and @tbg)

pkg/sql/logictest/testdata/logic_test/builtin_function, line 2921 at r3 (raw file):

Previously, angelapwen (Angela P Wen) wrote…

I wanted to write a logic test for the schema of the built-in but I'm not sure I can simply pass in 0 as the span — it is possible that a span's ID is 0 right?

Very unlikely, but yes. We could instead try using a negative number, which is definitely not possible.

pkg/sql/logictest/testdata/logic_test/contention_event, line 62 at r3 (raw file):

) SELECT count(*) > 0
  FROM payloads
  WHERE payload_type = '"type.googleapis.com/cockroach.roachpb.ContentionEvent"';

I wonder if we should strip out everything but roachpb.ContentionEvent. Seems like gunk that would be visible everywhere.

pkg/sql/sem/builtins/generator_builtins.go, line 344 at r3 (raw file):

			payloadsForSpanGeneratorType,
			makePayloadsForSpanGenerator,
			"Returns the payload(s) of the span whose ID is passed in the argument.",

Returns the payload(s) for the requested span.

pkg/sql/sem/builtins/generator_builtins.go, line 1396 at r3 (raw file):

// over all recordings for a given Span.
type payloadsForSpanGenerator struct {
	// The span to iterate over, set by the constructor.

No need to mention that it's set by the constructor.

pkg/sql/sem/builtins/generator_builtins.go, line 1399 at r3 (raw file):

	span *tracing.Span

	// recordingIndex maintains the current position of the index of the iterator

I understand that we're not iterating through all child recordings at the very beginning to try and be a bit more "stream-ey" about it, but does that actually help us with anything? We already have a handle on the tracing.Span, which already has all the payloads accessible to it in-memory. So it's already been allocated somewhere. What's the downside of simply slurping up everything the very first time and flattening it out in payloads?

(Asking more for my own understanding. The PR as written also obviously works, but I didn't realize there was a need to do anything fancy here.)

pkg/sql/sem/builtins/generator_builtins.go, line 1426 at r3 (raw file):

	spanID := uint64(*(args[0].(*tree.DInt)))
	span, found := ctx.Settings.Tracer.GetActiveSpanFromID(spanID)
	// A span may not be found if its ID was surfaced previously but its

Or it may just be a made up ID, we can skip saying anything here.

angelapwen

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @knz, and @tbg)

pkg/sql/logictest/testdata/logic_test/builtin_function, line 2921 at r3 (raw file):