Add vparquet4 encoding #3368

stoewer · 2024-02-06T07:25:30Z

What this PR does:
This adds a new encoding vParquet4 to Tempo (the default encoding remains unchanged). The new encoding improves support for links, events, and attributes with array values. This is a prerequisite better support of links, events, and arrays in TraceQL as proposed by the TraceQL Extensions design proposal.

The vParquet4 encoding is considered experimental for now.

There are still some things missing from this PR, those will be subject to separate PRs:

A CLI to analyze blocks in order to find good candidates for dedicated attributes
Documentation about how to enable vParquet4

Hints for reviewers: This PR was created from the commits to the vparquet3 directory in the branch main-vparquet4 (diff). Since this PR adds a whole new directory vparquet4, it can be helpful to review the individual PRs in order to see the differences between vParquet3 and vParquet4.

Which issue(s) this PR fixes:
Fixes grafana/tempo-squad#384

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

ie-pham · 2024-02-07T20:25:32Z

Ingester and querier memory usage is up

At this point vParquet3 and vParquet4 are identical

* Count dropped attributes and store them in separate column * Remove current support for arrays and kv-lists

* Store per-trace service statistics in vparquet4 blocks * Update pkg/traceql/combine.go * Update modules/frontend/search_progress.go * Update pkg/traceql/engine.go * Pre-allocate finalSpanset.ServiceStats * drop MaxServiceStats limit --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com> Co-authored-by: A. Stoewer <adrian@stoewer.me>

* Fix skipping of dropped attributes The attribute dropping introduced in grafana#3123 wans't implemented correctly * Implement single values as arrays and introduce type column * Add support for array attributes * Tests more array values and fix attribute conversion * Replace individual pointer helper functions * Use a combination of snappy and delta for ValueType * Vendor module google/go-cmp/cmp/cmpopts * Use all types of array attributes in TestTraceToParquet * Improve TestFieldsAreCleared

* links and events schema changes * update tests * update test * lint * fixed tests after rebase * lint * addressed comments * lint version * reduce span to just start time for events conversion

* Add attributes to instrumentation scope Attributes are not filled as they are not supported in current proto * Populate instrumentation scope in test data * Use delta encoding for dropped attributes count columns * More consistent naming

The test use values are tuned to current blocks from ops in order to have correct matching / non-matching queries

joe-elliott

Adding comments, but I'm not 100% done. I'm quite close and only have the second half of schema.go left

.golangci.yml

CHANGELOG.md

modules/frontend/search_progress.go

pkg/traceql/combine.go

pkg/traceql/engine.go

tempodb/encoding/vparquet4/schema.go

tempodb/encoding/vparquet4/block_traceql.go

tempodb/encoding/vparquet4/combiner.go

tempodb/encoding/vparquet4/schema.go

joe-elliott

I've made a full pass of the PR now and all concerns are currently in unresolved conversations.

…umns

* Fix combining service stats by using max() * update comment Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

stoewer · 2024-02-16T01:42:29Z

@andreasgerstmayr thanks for submitting the PRs

#3) * Recalculate ServiceStats in combiner by moving it to assignNestedSetModelBounds() * add additional tests Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

* Move serviceStats iterator to second pass * fix nil map assignment Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

read (grafana#3499)

…r throughput (grafana#3579)

joe-elliott

This was well reviewed a month or so back. A few questions and thoughts but this is quite close.

tempodb/encoding/vparquet4/block_traceql.go

pkg/traceql/storage.go

tempodb/encoding/vparquet4/schema_test.go

joe-elliott · 2024-04-19T12:59:14Z

tempodb/encoding/vparquet4/schema.go

+
+func attrToParquetTypeUnsupported(a *v1.KeyValue, p *Attribute) {
+	jsonBytes := &bytes.Buffer{}
+	_ = jsonMarshaler.Marshal(jsonBytes, a.Value) // deliberately marshalling a.Value because of AnyValue logic


should we do proto marshalling here for perf?

We use json for ValueKVList and ValueArray in vParquet3. Those columns are mostly empty and therefore I don't expect a big performance impact here.

Using JSON has the advantage that it is a bit easier to inspect those columns with parquet-cli

looks like we swapped span links and event attributes to proto but left others as json?

https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet3/schema.go#L558
https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet3/schema.go#L671

i'm fine with leaving these as json. likely they will be rare. we should keep an eye on profiles as we roll this out to see if there is significant impact on resource usage.

tempodb/encoding/vparquet4/schema.go

pkg/traceql/enum_attributes.go

joe-elliott · 2024-04-23T19:17:08Z

@stoewer @andreasgerstmayr this is very close. thank you for your work. there are 3 threads left to resolve. two of which are covered by andreas's PR: stoewer#5

if we push on this i think we can get it quite soon. let me know when you're ready for a final review 🙏

* make ServiceStats an intrinsic * skip allocating ServiceStats in rebatchIterator if already allocated Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

tempodb/encoding/vparquet4/block_traceql.go

The regression was introduced by making ServiceStats an intrinsic

Use the same queries in vp3 and vp4

* Copy vparquet3 to vparquet4 folder * Create initial vParquet4 encoding * vParquet4: handle unsupported / dropped attributes (grafana#3123) * Count dropped attributes and store them in separate column * Remove current support for arrays and kv-lists * Store per-trace service statistics in vparquet4 blocks (grafana#2941) * Add support for attributes with array values (grafana#3221) * Implement single values as arrays and introduce type column * Add support for array attributes * Tests more array values and fix attribute conversion * Use a combination of snappy and delta for ValueType * Vendor module google/go-cmp/cmp/cmpopts * Use all types of array attributes in TestTraceToParquet * Improve TestFieldsAreCleared * links and events schema changes (grafana#3163) * Add attributes to instrumentation scope (grafana#3322) * Fix typo in vParquet4 event name enconding (grafana#3336) * Precalculate and reuse the vParquet4 schema before opening blocks * Convert block in vparquet4/test-data directory * CHANGELOG.md * Update test cases in BenchmarkBackendBlockTraceQL * Unsupported attribute values are no longer dropped * Replace ValueType column with IsArray column * Skip service stats map allocation when none are present --------- Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com> Co-authored-by: Andreas Gerstmayr <agerstmayr@redhat.com> Co-authored-by: Jennie Pham <94262131+ie-pham@users.noreply.github.com> Co-authored-by: Andreas Gerstmayr <andreas@gerstmayr.me>

stoewer force-pushed the add-vparquet4-encoding branch from 6ff9743 to feb53f0 Compare February 6, 2024 23:46

stoewer changed the title ~~Add vparquet4 encoding [wip]~~ Add vparquet4 encoding Feb 7, 2024

stoewer and others added 12 commits February 8, 2024 10:52

Copy vparquet3 to vparquet4 folder

444a6fc

Create initial vParquet4 encoding

31140a4

At this point vParquet3 and vParquet4 are identical

vParquet4: handle unsupported / dropped attributes (grafana#3123)

9775501

* Count dropped attributes and store them in separate column * Remove current support for arrays and kv-lists

links and events schema changes (grafana#3163)

794fb93

* links and events schema changes * update tests * update test * lint * fixed tests after rebase * lint * addressed comments * lint version * reduce span to just start time for events conversion

Fix typo in vParquet4 event name enconding (grafana#3336)

a2e10bb

Precalculate and reuse the vParquet4 schema before opening blocks

9024d10

Address linter warnings

06521ed

Convert block in vparquet4/test-data directory

1f46601

CHANGELOG.md

580d50f

stoewer force-pushed the add-vparquet4-encoding branch from 72c01b2 to 580d50f Compare February 8, 2024 01:00

Update test cases in BenchmarkBackendBlockTraceQL

16432c9

The test use values are tuned to current blocks from ops in order to have correct matching / non-matching queries

joe-elliott reviewed Feb 13, 2024

View reviewed changes

stoewer marked this pull request as ready for review February 14, 2024 00:29

stoewer requested review from annanay25, mdisibio, mapno, kvrhdn, zalegrala, electron0zero and ie-pham as code owners February 14, 2024 00:29

joe-elliott reviewed Feb 14, 2024

View reviewed changes

stoewer added 3 commits February 15, 2024 10:52

Unsupported attribute values are no longer dropped

156ba47

Merge remote-tracking branch 'upstream/main' into add-vparquet4-encoding

1f6708b

Backport: Update traceql metrics to use the trace-level timestamp col…

8ec97c9

…umns

Fix combining service stats by using max() (#1)

9393edf

* Fix combining service stats by using max() * update comment Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

andreasgerstmayr mentioned this pull request Feb 16, 2024

Move serviceStats iterator to second pass stoewer/tempo#4

Merged

3 tasks

andreasgerstmayr added 2 commits April 10, 2024 11:19

Recalculate ServiceStats in combiner by moving it to separate function (

d79cea6

#3) * Recalculate ServiceStats in combiner by moving it to assignNestedSetModelBounds() * add additional tests Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Move serviceStats iterator to second pass (#4)

9ab5514

* Move serviceStats iterator to second pass * fix nil map assignment Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

stoewer force-pushed the add-vparquet4-encoding branch from a1bbc66 to 9ab5514 Compare April 12, 2024 07:23

stoewer added 11 commits April 15, 2024 16:20

Merge remote-tracking branch 'upstream/main' into add-vparquet4-encoding

9bef5e8

Backport: Better trace ID sharding (grafana#3399)

c89e606

Backport: Add string interning to SyncIterator (grafana#3411)

9b81152

Backport: Traceql better pooling (grafana#3432)

66a54e8

Backport: New (unsafe) query hints (grafana#3396)

ff97bb8

Backport: Redo 8-byte trace ID sharding to reduce number of row groups

dc10143

read (grafana#3499)

Backport: TraceQL nested set intrinsics (grafana#3497)

0587418

Backport: Parquetquery iterator less pooling (grafana#3508)

ad936e3

Remove searchProgress handler

665b765

Fix tests: Unset service stats in TraceSearchMeta

3627f3d

Backport: Fix pool leak when iter exhausted and async pages for bette…

0c139e9

…r throughput (grafana#3579)

joe-elliott reviewed Apr 19, 2024

View reviewed changes

make ServiceStats an intrinsic (#5)

19dbeab

* make ServiceStats an intrinsic * skip allocating ServiceStats in rebatchIterator if already allocated Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

joe-elliott reviewed Apr 29, 2024

View reviewed changes

tempodb/encoding/vparquet4/block_traceql.go Show resolved Hide resolved

stoewer added 4 commits April 30, 2024 19:19

Replace ValueType column with IsArray column

2ef85b3

Skip service stats map allocation when none are present

d290290

Fix performance regression in existing parquet encodings

a8abcb5

The regression was introduced by making ServiceStats an intrinsic

Merge remote-tracking branch 'upstream/main' into add-vparquet4-encoding

8108e03

joe-elliott approved these changes May 6, 2024

View reviewed changes

stoewer added 2 commits May 7, 2024 08:59

Merge remote-tracking branch 'upstream/main' into add-vparquet4-encoding

393b2a7

Use same queries in BackendBlockTraceQL

8c71ae5

Use the same queries in vp3 and vp4

stoewer merged commit fc33ae5 into grafana:main May 7, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vparquet4 encoding #3368

Add vparquet4 encoding #3368

stoewer commented Feb 6, 2024 •

edited

Loading

ie-pham commented Feb 7, 2024

joe-elliott left a comment

joe-elliott left a comment

stoewer commented Feb 16, 2024

joe-elliott left a comment

joe-elliott Apr 19, 2024

stoewer Apr 26, 2024

joe-elliott Apr 29, 2024

joe-elliott commented Apr 23, 2024

Add vparquet4 encoding #3368

Add vparquet4 encoding #3368

Conversation

stoewer commented Feb 6, 2024 • edited Loading

ie-pham commented Feb 7, 2024

joe-elliott left a comment

Choose a reason for hiding this comment

joe-elliott left a comment

Choose a reason for hiding this comment

stoewer commented Feb 16, 2024

joe-elliott left a comment

Choose a reason for hiding this comment

joe-elliott Apr 19, 2024

Choose a reason for hiding this comment

stoewer Apr 26, 2024

Choose a reason for hiding this comment

joe-elliott Apr 29, 2024

Choose a reason for hiding this comment

joe-elliott commented Apr 23, 2024

stoewer commented Feb 6, 2024 •

edited

Loading