Add opencensus integration #396
Conversation
googlebot
added
the
cla: yes
|
Hi @taddari. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
|
@taddari: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@taddari try again - added you to the triage permission which should let you control the bot |
pkg/database/database.go
Outdated
| // OpenWithCacher creates a database connection with the cacher. This should | ||
| // only be called once. | ||
| func (db *Database) OpenWithCacher(ctx context.Context, cacher cache.Cacher) error { | ||
| c := db.config | ||
|
|
||
| rawDB, err := gorm.Open("postgres", c.ConnectionString()) | ||
| driver := ocsql.Wrap(&postgres.Driver{}, ocsql.WithAllTraceOptions()) |
There was a problem hiding this comment.
Two things here:
-
This feels like a lot of code, and I'm not entirely sure why it's all necessary. Can we just register the callback?
-
I'm concerned that this observability might allow a server operator to correlate specific database events (e.g. an insert) which a TEK upload. It's the same problem as the network observer, but since our chaff requests don't hit the database, it would be easy to separate chaff from non-chaff /cc @mikehelmick
There was a problem hiding this comment.
Hi Seth,
About your first point, the callbacks will actually emit only gorm specific metrics. https://github.com/sagikazarmark/go-gin-gorm-opencensus/blob/master/pkg/ocgorm/stats.go.
For postgres specific metrics, we need to override the driver. Following are the metrics that are emitted by https://github.com/opencensus-integrations/ocsql#recorded-metrics.
For point 2, I think we should disable tracing in that case and that should prevent the server operator to correlate anything.
There was a problem hiding this comment.
I have changed the code to remove traceability. WDYT?
There was a problem hiding this comment.
I think we're gonna need to hold off until we hear from the privacy team (I pinged them).
There was a problem hiding this comment.
Also worth remembering, we're sampling traces at 40%.
There was a problem hiding this comment.
@sethvargo any update from the privacy team?
|
/ok-to-test |
# Conflicts: # pkg/database/database.go
|
/lgtm Fix merge conflict tho. |
# Conflicts: # go.mod # go.sum
|
/retest |
|
Done the changes. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: icco, taddari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #267
Proposed Changes
Release Note