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

opencensus inbuilt support #853

Closed
despondency opened this issue Oct 21, 2020 · 12 comments
Closed

opencensus inbuilt support #853

despondency opened this issue Oct 21, 2020 · 12 comments
Labels
needs response Needs a response from the issue/PR author

Comments

@despondency
Copy link

despondency commented Oct 21, 2020

Currently opencensus (ocsql) works only through sql.DB, however this way we miss on too many features in pgx, could we somehow use pgx, but not need to go lower with sql.DB? Is there any way?

@jackc
Copy link
Owner

jackc commented Oct 24, 2020

I'm not familiar with opencensus, but after a quick look at there page I think the situation is as follows:

You should be able to use ocsql.Wrap or ocsql.Register along with pgx's stdlib.RegisterConnConfig to configure any pgx settings that can't be set through the connection string.

As far as using pgx specific functionality such as CopyFrom, I'm not sure if it is possible. Ordinarily, you could use db.Conn.Raw() or stdlib.AcquireConn to get a pgx connection. Not sure if that would work with whatever wrapping ocsql is doing but I suspect db.Conn.Raw would not work, but stdlib.AcquireConn would.

Once you had a pgx *Conn you could use whatever pgx features you wanted, but whatever metric tracking you were doing with opencensus would not see it.

@despondency
Copy link
Author

Using batched queries is another functionality that cannot be done with database/sql. Could we assist/request a pluggable metrics implementation using opencensus? Since opencensus is used and adopted quite widely, its a shame that you can track everything up to the DB layer and then be left in the dark what and how long is taking when we enter the DB.

Another reason why going down to sql.DB is a bad idea is that once again you go back to the text protocol rather than using pgx's binary. Also pgx employs some pretty strong invariants regarding column types in DB being mapped to golang struct types (which is good, but also painful if you've got wrong types in db -> DB has _text, and you try to get it into []UUID, while in fact it wants []string)

@jackc
Copy link
Owner

jackc commented Oct 25, 2020

Again, I am unfamiliar with opencensus so I don't know what know what integration with it quite entails, but I can think of two possibilities.

pgx has custom logging hooks. It might be that a logger could be implemented that emitted the desired metrics though since it was designed from the perspective of logging not metrics so it may not.

Otherwise, I think that wrapping a few core types such as pgx.Conn and pgxpool.Pool would allow for any metrics integration.

@despondency
Copy link
Author

https://github.com/opencensus-integrations/ocsql/blob/master/driver.go similar to this, everything is wrapped so it can create spans when you query/exec/ etc.

Can we do something like this to be pluggable for pgx?

@jackc
Copy link
Owner

jackc commented Oct 26, 2020

That would be the approach of wrapping the pgx types -- and yes I think it is possible.

@despondency
Copy link
Author

Can we open a feature request, or contribute in some way? Do you have any guidelines how exactly should we go about this?

@jackc
Copy link
Owner

jackc commented Oct 27, 2020

Can we open a feature request

You can leave this issue open, but I don't know of anyone aside from you who might be planning on working on it.

or contribute in some way?

As I said above, I think the best approach would be to wrap the pgx types. That probably could be done without any pgx modifications. If so, then it probably would make sense as a separate project (or part of opencensus itself).

@fredbi
Copy link

fredbi commented Jun 10, 2021

@despondency the ocslq lib just wraps the driver. Here is an example on how I register the pgx driver (I am using *sqlx.DB on top of a pgx driver):


func (r Repository) open(dcfg *pgx.ConnConfig) (*sqlx.DB, error) {
        db := stdlib.OpenDB(*dcfg)

 
        driverName := "pgx"
        opts := r.config.TraceOptions(dcfg.ConnString())
        if len(opts) > 0 {
                // opencensus tracing registered in the sql driver
                // (this wraps the sql driver with an instrumented version)
                driverName, err = ocsql.Register(driverName, opts...)
                if err != nil {
                        r.zlg.Error("failed to register trace driver", zap.Error(err))
                        return nil, err
                }
        }

        // connection pool settings
        r.config.SetPool(db)

        return sqlx.NewDb(db, driverName), nil
}

...
// TraceOptions returns the trace options for the opencensus driver wrapper
func (r config) TraceOptions(u string) []ocsql.TraceOption {
        if !r.cfg.GetBool(cfgPGTraceEnabled) {
                return nil
        }
        v, _ := url.Parse(u)

        return append(sqlDefaultTraceOptions(), ocsql.WithInstanceName(v.Redacted()))
}

func sqlDefaultTraceOptions() []ocsql.TraceOption {
        // _almost_ WithAllTraceOptions: just remove the WithRowsNext and Ping which produce a lot of clutter in traces
        return []ocsql.TraceOption{
                ocsql.WithAllowRoot(true),
                ocsql.WithLastInsertID(true),
                ocsql.WithQuery(true),
                ocsql.WithQueryParams(true),
                ocsql.WithRowsAffected(true),
                ocsql.WithRowsClose(true),
        }
}

@knusbaum
Copy link

knusbaum commented Jun 22, 2021

Wrapping the pgx types would be a little brittle.
It would be preferable from a usability and maintainability standpoint to have some way to install middleware, or some interface that could be implemented.

Would you consider implementing one of:

  • hooks
  • middleware
  • interface

to allow things like tracers to inspect the behavior of pgx?

@jackc
Copy link
Owner

jackc commented Jun 26, 2021

If some sort of clean abstraction can be devised that enables this I would be interested in discussing it (and please do discuss before coding), but as I haven't personally needed this functionality I don't have the context or motivation to design or implement it myself.

@bfontaine
Copy link
Contributor

Is somebody working on this? Does it make sense to keep this issue open?

@bfontaine bfontaine added the needs response Needs a response from the issue/PR author label Nov 7, 2022
@jackc
Copy link
Owner

jackc commented Nov 12, 2022

I think the addition of tracing hooks in pgx v5 should enable this integration by a 3rd party. AFAIK, no further changes in pgx are required.

@jackc jackc closed this as completed Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Needs a response from the issue/PR author
Projects
None yet
Development

No branches or pull requests

5 participants