SQLDriverSegmentBuilder containing ParseArgs func

[psimoesSsimoes]

Summary

At the moment, datastore segments created by pre-built integration packages (for MySQL, PostgreSQL, or SQLite database drivers) never set segments.QueryParameters.

Desired Behaviour

If SQLDriverSegmentBuilder contained an ParseArgs func field func then ExecContext, QueryContext, and QueryRowContext implementations would call this func to set queryparameters.

Since some query parameters might be sensitive, it should be optional for the client to use this ParseArgs when calling these implementations.

Possible Solution

we would add parse args to SQLDriverSegmentBuilder:

type SQLDriverSegmentBuilder struct {
   BaseSegment DatastoreSegment
   ParseQuery  func(segment *DatastoreSegment, query string)
   ParseDSN    func(segment *DatastoreSegment, dataSourceName string)
   ParseArgs    func(segment *DatastoreSegment, args []driver.NamedValue)
}

On QueryContext, and ExecContext we would call it after segment creation.

One example, using wrapStmt.QueryContext:

// QueryContext implements StmtQueryContext.
func (w *wrapStmt) QueryContext(ctx context.Context, args []driver.NamedValue) (driver.Rows, error) {
    segment := w.bld.startSegment(ctx)
    rows, err := w.original.(driver.StmtQueryContext).QueryContext(ctx, args)
    w.bld.ParseArgs(&segment,args)
        segment.End()
    return rows, err
}

We could support two parse args implementations:
NopParseArgs would be the default value and it´s implementation would be a NOP.

ParseArgs implementation could be:

package sqlparse

 import (
   "database/sql/driver"
   "fmt"

    newrelic "github.com/newrelic/go-agent/v3/newrelic"
 )

 //`NopParseArgs` would be the default value and it´s implementation would be a NOP.
func NopParseArgs(segment *newrelic.DatastoreSegment, args []driver.NamedValue) {
}

// `ParseArgs` would set query parameters using ordinal (since name might be empty)
func ParseArgs(segment *newrelic.DatastoreSegment, args []driver.NamedValue) {
   if segment == nil || len(args) == 0 {
	return
   }

   if segment.QueryParameters == nil {
	segment.QueryParameters = map[string]interface{}{}
   }

   for _, arg := range args {
	 segment.QueryParameters[fmt.Sprintf("%d", arg.Ordinal)] = arg.Value
   }
}

Packages v3/integrations/nrmysql ,v3/integrations/nrpq and 3/integrations/nrsqlite3 by default would use the NopParseArgs.

baseBuilder = newrelic.SQLDriverSegmentBuilder{
	BaseSegment: newrelic.DatastoreSegment{
		Product: newrelic.DatastoreMySQL,
	},
	ParseQuery: sqlparse.ParseQuery,
	ParseArgs:  sqlparse.NopParseArgs,
	ParseDSN:   parseDSN,
}

This is the part that is not clear to me: how could a client set baseBuilder ParseArgs if it needed?

Additional context

We don't get much information about the queries parameters which help to debug queries that are slow for some query parameters and not for others.

[psimoesSsimoes]

Or... If there was a way to access app config via context on ParseArgs, we could only expose args if some conf was previously set.

[nr-swilloughby]

Thanks for the suggestion! We're already considering an enhancement like this for reporting more details of SQL queries than what the agent already does. We'll make sure to consider these features, or something equivalent, in that update.

[psimoesSsimoes]

Great news!

Would you guys accept/want any help implementing that update?

I would love to participate on it, if possible 😁

[iamemilio]

Hi @psimoesSsimoes if you wanted to take a shot at it and submit a PR, that would be greatly appreciated! We would be happy to provide support or feedback.

[psimoesSsimoes]

@iamemilio i will do it.

One question before i draft it:
In your opinion, should we always show query args by default?
Or given the possibility of sensive nature of some query args, we should only show them if a client explicitly tell us to do so?

[iamemilio]

Good question! In my opinion, we should exclude them by default, since most customers will not know that new functionality has been added and expect the behavior which protects their data.

[jimmycdinata]

@psimoesSsimoes any update on this?