[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.
|
| Comments |
| Comment by Jeffrey Yemin [ 03/Apr/19 ] | ||||||||||
|
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:
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
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:
I hope that's clearer.
| ||||||||||
| Comment by Jeffrey Yemin [ 14/Mar/19 ] | ||||||||||
|
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. |