[GODRIVER-837] Provide a means of associating data with a monitored event Created: 19/Feb/19  Updated: 03/Apr/19  Resolved: 03/Apr/19

Status: Closed
Project: Go Driver
Component/s: Monitoring
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Andrew Wilkins Assignee: Jeffrey Yemin
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

The event monitoring API currently provides a means of accepting context, but does not provide a means of storing request-specific context.

The driver already maintains a map, but does not expose it in any way to users. Any monitoring code that wishes to associate a request's started and finished events must therefore maintain its own map.

Ideally the event monitoring API would provide a method for storing and retrieving arbitrary data against the request. This could be added in a backwards-compatible manner by utilising the existing context parameter, e.g.

 

package event
 
func SetValue(ctx context.Context, key, value interface{})
func GetValue(ctx context.Context, key interface{}) (interface{}, bool)

 



 Comments   
Comment by Jeffrey Yemin [ 03/Apr/19 ]

axw

Will do. Let us know if and when you get back to this, and we'll be happy to reconsider.

Comment by Andrew Wilkins [ 03/Apr/19 ]

Thanks, @Jeff Yemin.

Fair point about network; the map lookups would indeed be dwarfed. Memory usage and lock contention may still be an issue, but it is somewhat hypothetical.

I'm happy to look into it myself, but I don't expect I'll get to it any time soon. Feel free to close this for now if you don't want it hanging around.

Thanks for your time.

Comment by Jeffrey Yemin [ 02/Apr/19 ]

Hi axw

Thanks for the additional information. I understand that you're trying to reduce overhead as much as is feasible, but keep in mind that every command involves a network round-trip, work on the server to execute the command as well as BSON serialization and deserialization on the client. Given that there is a way to accomplish your goal with the current API, we would need to ascertain that the proposed improvement gives enough bang for the buck, so to speak.

Let me know if you're willing to take on the work of patching the driver and measuring the performance change between the current API and a proposed new API. If you're able to measure a significant improvement, we can consider next steps.

Comment by Andrew Wilkins [ 15/Mar/19 ]

Jeff, thanks for the reply.

I should have provided a bit more context in the first place. I work on the Elastic APM Go agent, and have recently implemented a tracing module for the MongoDB Go Driver. The code is here: https://github.com/elastic/apm-agent-go/blob/master/module/apmmongo/monitor.go. We're already storing context in a map using the connection and request IDs as a key. This works, but the map introduces overhead: locking, additional memory usage, lookups.

The way this module is used is something like this:

 

var client *mongo.Client
 
func init() {
    client, _ = mongo.Connect(context.Background(), options.Client().SetMonitor(apmmongo.CommandMonitor()))
}
 
func myHandler(w http.ResponseWriter, req *http.Request) {
    _ = client.Database("dbname").RunCommand(req.Context(), bson.D{/*...*/})
}

As long as the context contains an apm.Transaction we'll trace the MongoDB command. So we could add a map of arbitrary key/value pairs on our Transaction object, but that doesn't help much. The overhead mentioned above would just have moved from the CommandMonitor to the Transaction object.

> Go's Context API allows you to associate arbitrary keys and values, so it would be possible for your application to do that itself with the Context

I think ideally the `CommandMonitor.Started` field should have the signature

func(context.Context, *CommandStartedEvent) context.Context

with the context returned passed into CommandMonitor.Succeeded/Failed. That way we could pass state along in the cheapest way possible. Given that 1.0.0 just happened, that ship has sailed. A couple of alternatives that I can see are:

  • Deprecate CommandMonitor.Started, and add a separate method CommandMonitor.Started2 (just for illustration; pick a better name) which returns a context as described above. If Started2 is non-nil, call that instead of Started.
  • Add a field `State interface{}` to CommandStartedEvent and CommandFinishedEvent, the latter's value being set to the output value of the former.

I hope that's clearer.

 

Comment by Jeffrey Yemin [ 14/Mar/19 ]

axw

You should be able to associated started and finished/failed events utilizing the requestId field in the respective event structs. These should be unique across all events, as the driver maintains a single monotonically increasing counter for the request id.

If you need something more than that, Go's Context API allows you to associate arbitrary keys and values, so it would be possible for your application to do that itself with the Context that it passes to some CRUD API method in the driver.

Comment by Andrew Wilkins [ 19/Feb/19 ]

Alternatively, just add a map to the event structs, copying it from the started to finished event. This would have the added benefit of enabling monitoring code to be able to work with the old and new versions, albeit requiring reflection.

Generated at Thu Feb 08 08:35:04 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.