CASSGO-119 Add Table field to ObservedQuery and ObservedBatch for table-level observability

[raj14243]

ObservedQuery and ObservedBatch did not expose the target table name, requiring consumers (metrics collectors, loggers, tracers) to parse the CQL statement string to determine which table a query targets.

This change adds a Table field to ObservedQuery and a Tables field to ObservedBatch, populated from prepared statement metadata returned by Cassandra. The table information was already available internally via preparedMetadata (used for routing), so no additional round-trips or CQL parsing are needed.

Patch by Raj Ummadisetty for CASSGO-119

[worryg0d]

Hi @raj14243,

I have spent some time reviewing this PR - LGTM.

One thing I noticed is that PR lacks integration testing for this change. This is a small change actually, but updating existing tests would be good anyway. There are TestObserve and TestBatchObserve integration tests for both query and batch observe respectively, that you can update to reflect this API change.

[worryg0d]

I think it is fine to populate the existing Keyspace field here - it is unclear to see the difference between Keyspace and StatementKeyspace

[raj14243]

The existing Keyspace field is populated with Keyspace the session is initialized with. I am concerned changing that might break if someone is using it with that assumption.

[worryg0d]

Yeah, this is why I filed CASSGO-120, I'm unsure if this is the correct behavior. At least I was expecting it to be based on the query, if it is presented there

[worryg0d]

routingInfo could not have any information about keyspaces/tables for DDL queries that are unprepareable, so you probably should use internalQuery.Keyspace() here. It has some predefined defaulting behavior.

We should probably think about adding a kind of parsing of DDL queries to determine keyspace and table names from it before defaulting to session-level keyspace.

Also, there is an alternative for internalBatch.Keyspace() as well, but it just calls the underlying routingInfo, which is fine because it is impossible to batch DDL queries.

@joao-r-reis, could you please take a look at this?

[joao-r-reis]

Hmm I don't think we should be adding CQL parsing for something like this, most if not all drivers avoid doing any kind of CQL parsing. What some drivers do is expose a statement builder / mapper API that will create statements and internally set fields so the driver is aware of some metadata like keyspace, table, idempotency without having to parse it.

[raj14243]

I will check the internalQuery.Keyspace() I agree with @joao-r-reis on not having CQL parsing. I started this PR as we were internally parsing statement to extract keyspace and wanted to avoid that :)

[joao-r-reis]

I think the current design is not intuitive for cases where no prepared statement metadata is available.

I explored a bit and came up with something like this:

type PreparedMetadata struct {
    Keyspace string
    Table    string
}

type ObservedQuery struct {
    Keyspace          string
    Statement         string
    PreparedMetadata  PreparedMetadata  // zero value when unprepared
    IsPrepared        bool              // flag at query level
    // ...
}

type ObservedBatch struct {
    Keyspace          string
    Statements        []string
    PreparedMetadata  []PreparedMetadata  // zero values for unprepared
    IsPrepared        []bool              // parallel array of flags
    // ...
}

We could also just get rid of the IsPrepared flag and just make prepared metadata a pointer (so nil would mean it's not prepared) but that would lead to extra allocs.

Wdyt?

[joao-r-reis]

Add a new 2.2.0 section and move this under there

[worryg0d]

Wdyt?

Yeah I guess it would work, but API looks a bit gross. As alternative to exposing those fields to ObservedXY structs, you may call this method:

cassandra-gocql-driver/session.go

Line 756 in c65c762

func (s *Session) StatementMetadata(ctx context.Context, stmt, keyspace string) (StatementMetadata, error) {

It basically allows to retrieve internally cached statement metadata. But this requires observers to be able to call session methods

[worryg0d]

Besides CASSGO-120, this PR looks good.

I guess you can get rid of CASSGO-120 in the commit message, and we will handle it separately if it is worth fixing

[joao-r-reis]

Let's hold off on merging this until 2.1.2 is released

[worryg0d]

+1

[joao-r-reis]

@raj14243 can you add both of us as reviewers to the commit message and fix the git conflict? Will merge afterwards

[joao-r-reis]

Or if you don't mind github not recognizing the PR as "merged" then I can do that for you and set you as author of the commit

[raj14243]

@raj14243 can you add both of us as reviewers to the commit message and fix the git conflict? Will merge afterwards

@joao-r-reis done