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

session: include partition key in RequestSpan in human readable form #766

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

piodul
Copy link
Collaborator

@piodul piodul commented Jul 21, 2023

This PR changes the format used by RequestSpan to represent the partition key for a given query. Previously, it was the same representation that the driver creates before applying MurmurHash3 to it, encoded in base64 - the format is documented so technically users can decode it themselves, but it's quite inconvenient. Now, the driver decodes the contents of the partition key and represents it in a human-readable form, similar to the CQL syntax.

For example, given the schema

create table ks.t (pk1 int, pk2 time, pk3 frozen<set<text>>, primary key(pk1, pk2, pk3));

and the request

session
    .execute(
        &prepared,
        (
            6622i32,
            Time(chrono::Duration::seconds(12323)),
            vec!["quick", "brown", "fox"],
        ),
    )
    .await
    .unwrap();

previously, the encoded key would look like this:

partition_key=0004000019de00000800000b352c099e0000001d0000000300000005717569636b0000000562726f776e00000003666f7800

but now it looks like this:

partition_key=6622,'03:25:23.000000000',{'quick','brown','fox'}

Fixes: #720

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@piodul piodul requested review from cvybhu and wprzytula July 21, 2023 13:51
@piodul piodul force-pushed the pretty-print-routing-key-in-tracing branch 2 times, most recently from 25abcbf to a262b2f Compare July 21, 2023 14:01
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, only some naming/comments nits.

scylla/src/utils/pretty.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
piodul added 3 commits July 24, 2023 18:19
A pretty printer for CqlValues is added. The intent is to use it in
tracing, notably for including information about requests' partition
keys.
Adds a utility structure that allows to recover CqlValues from an
encoded partition key (also called a routing key). It will be used for
including requests' partition keys in a human-readable form in tracing.
The RequestSpan is modified so that it includes the partition key in a
human readable form instead of the encoded one.
@piodul piodul force-pushed the pretty-print-routing-key-in-tracing branch from a262b2f to a95ed83 Compare July 24, 2023 16:23
@piodul
Copy link
Collaborator Author

piodul commented Jul 24, 2023

v2:

  • std::iter::chain instead of collect to vector then push,
  • Rename idx -> col_idx,
  • Rename index -> value_index,
  • Added a link to the explanation of the format that PartitionKeyDecoder decodes from,
  • Reworked the tests for PartitionKeyDecoder a bit and added a more complicated case that involves out-of-order columns and different column types.

@piodul piodul requested a review from wprzytula July 26, 2023 07:11
@wprzytula wprzytula merged commit e73d417 into scylladb:main Jul 26, 2023
@wprzytula wprzytula mentioned this pull request Jul 28, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the partition key's representation in tokio tracing to a human-readable form
2 participants