DEVPROD-4511 Remove db logging from honeycomb #7940
Conversation
|
Added some comments and updated the description |
|
Sorry for the drive by review. But is it possible to reconsider this or keep it enabled for traces stemming from a graphql query? Having the db statement is invaluable for debugging performance issues or debugging problematic queries that can't be easily replicated. I was under the impression that refinery instance already redacts secrets from db queries? Cc @ybrill |
|
The application omits |
| @@ -368,7 +368,8 @@ func (e *envState) initDB(ctx context.Context, settings DBSettings, tracer trace | |||
| opts := options.Client().ApplyURI(settings.Url).SetWriteConcern(settings.WriteConcernSettings.Resolve()). | |||
| SetReadConcern(settings.ReadConcernSettings.Resolve()). | |||
| SetConnectTimeout(5 * time.Second). | |||
| SetMonitor(apm.NewMonitor(apm.WithCommandAttributeDisabled(false), apm.WithCommandAttributeTransformer(redactSensitiveCollections))) | |||
| // Removing WithCommandAttributeDisabled(false) to disable db command monitoring. | |||
There was a problem hiding this comment.
Comments should be a bit more general -- this is a bit more info about what happened in the PR, but in the future someone will look at this comment and say, "huh"? Perhaps "Command monitoring not disabled to avoid logging redacted secrets" or etc.
@ybrill do you have any suggestions for how to do what Mohamed is proposing, or how to securely log db.statement? We may need some advice on this, since this relates to honeycomb setup.
There was a problem hiding this comment.
I once thought a tenable middle ground could be to have the logging parse the db.statement as json and strip the leaf values (like the strings and whatever that don't have children). I never attempted to implement it, though.
Keeping it for just graphql traces could also work, though that would have to be in the collector, I think.
There was a problem hiding this comment.
I wonder if db.statement without any values would still be useful though?
I'm not super sure on what you mean by "would have to be in the collector" could I ask you to explain that part
There was a problem hiding this comment.
That's true: the query plan could be different with different values.
Sorry, for not being clear in the first place 😅. In the application, when we're making the span for the database call the only thing we know about our lineage is id of our parent and the overall trace id. So we don't have a way to know whether the trace we're part of is a graphql one. On the other hand, if we wanted to drop the attribute once the span arrives at the collector we could have the application add a flag attribute to all the child spans of a graphql request and when we see a db span with that flag then we'll know to keep the db.statement.
DEVPROD-4511
Description
don't log db statements in honeycomb because it could leak secrets
What removing WithCommandAttributeDisabled() does is set our monitor to the default behavior of no longer adding MongoDB commands as attributes
According to @ybrill , that was the only reason we use a copy of the open telemetry library in anser and we could go back to using the original if we don't plan on using this feature again. I don't believe that is necessary at the moment so no follow up tickets have been made. Similarly, no ticket has been created to re-enable this span after sanitizing our commands because it may not be necessary.
Testing
no longer logs db.statement in staging
