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

Mongo dbstats #3228

Merged
merged 38 commits into from Jan 11, 2017

Conversation

@scottcrespo
Copy link
Contributor

commented Dec 20, 2016

@ruflin

Opening the PR early so we have a place to discuss implementation. At this point, I've copied the patterns used by the status metricset, but I have some questions and recommendations. Keep in mind, I'm still learning about the platform, so I might be making some incorrect assumptions!

@elasticsearch-release

This comment has been minimized.

Copy link

commented Dec 20, 2016

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2016

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

func (m *MetricSet) Fetch() ([]common.MapStr, error) {

// establish connection to mongo
session, err := mgo.DialWithInfo(m.dialInfo)

This comment has been minimized.

Copy link
@scottcrespo

scottcrespo Dec 20, 2016

Author Contributor

@ruflin

This follows the pattern used by the status metricset here, but I'm not sure it makes sense to open and close the mongo connection each time fetch() is called.

According to the [docs],(https://www.elastic.co/guide/en/beats/metricbeat/current/metricset-details.html#_timeout_connections_to_services) connections should be established in the New() method and persist between fetches.

I'm going to begin implementing a pattern where the mongo session is persisted in each MetricSet instance to avoid constantly opening and closing connections. I would add that work to this existing PR. Or, would you prefer it's handled under a separate submission?

Lastly, based on this implementation I'm more confident that the mongodb module is not connecting directly to the individual instances, but would instead connect to the cluster. This means that any cluster member may answer the query and result in data inconsistencies. This issue is a bit more involved, and I don't know enough about the platform to be sure, so I may need to jump on a Zoom call or something to talk further.

This comment has been minimized.

Copy link
@ruflin

ruflin Dec 21, 2016

Collaborator

+1 on persisting the connection. It is probably best to extract the connection setup logic to the module level and just use it as a function.

About connecting to single instances or the cluster can be come tricky. We had a similar issue with Kafka. Our recommendation is to install metricbeat on each edge node (some exceptions exist here), as it gives you also additional metrics about the system etc. I'm don't know the details of the mongodb clustering but for Kafka we solved it the following way (generic description):

  • Individual node stats are reported by each mongo instance
  • Global stats about cluster / indices are only fetched from master

This requires that a metricbeat instance can detect if it is a master or not. Also I assume this must be dynamic, as master can change over time.

I assume dbStats response is the same for each node, so only the data from master should be fetched.

Happy to jump on a zoom call to go into more details.

This comment has been minimized.

Copy link
@scottcrespo

scottcrespo Dec 21, 2016

Author Contributor

@ruflin

Connecting to single instances

Tricky indeed. Fortunately, mgo (the mongo client) has a dialInfo.Direct parameter. In mongobeat, I initiate a master connection to the cluster, and then create individual node connections to each node. You can see it here

I create the individual connections in the "controller" here

I've begun implementing something similar for the mongodb module, minus node discovery. This is implemented at the module level to share logic and prevent redundant connections.

Detecting master/primary

Easily implemented with mgo. You can specify read preference to always read from primary. If the primary changes, your reads will automatically be directed to the new primary.

dbStats response the same

Technically dbStats response (and serverStatus) are not the same for each node. Sometimes the differences are trivial, other times they're not. I'll cover the cases where they're not the same:

replica sets

In the case of a replica set, the state of the secondaries lag behind the primary. Thus, seeing the stats for each node helps monitor replication lag and success/failure. This is especially helpful in the case of multi-datacenter replication, where larger discrepancies are usually present.

sharded clusters

In the case of a sharded cluster, the complexity is compounded because nodes contain different databases and collections, and a shard can also be a replica set! =o

Conclusion

Given these considerations, I'm still having a hard time understanding what the best implementation for metricbeat would be. Given the current implementation - the node that is read from is implicit and can change from period to period. For example, you might connect to mongo on localhost and expect that you're reading from that node. But under the hood, mgo is actually discovering the other nodes in the cluster and directing reads to any one of the member nodes. Thus, I think it's possible to receive unexpected and inconsistent results.

If we enforce that metricbeat only establishes a direct connection to a single mongo instance on localhost, then all of this complexity goes away and output is very predictable. Yay! But, it limits the flexibility of the system.

If we allow the agent to connect to mongo as a cluster, and permit configuration of multiple hosts, then I think there's a lot of different directions we could take.

Definitely a Zoom call will be helpful. I'm sure you can teach me a lot more about how metricbeat works under the hood, so I can understand the best direction.

I don't have your email, but can you contact me at sccrespo@gmail.com and we'll set up a time? Perhaps we could talk sometime Thursday afternoon (your time).

This comment has been minimized.

Copy link
@ruflin

ruflin Dec 23, 2016

Collaborator

@scottcrespo Sorry, I was mostly offline the last two days, so only saw the update now.

I always try to go with the simplest solution first, which I agree is the one only connecting to localhost. Because of docker I would probably rephrase this to: The mongodb module gets only the stats from the node it connects to, as it does not necessarly always have to be localhost on docker.

Could you update the PR so that it always only connects to the defined host (and not the cluster)?

About zoom call: Lets see how far we can take it here in the PR as I'm rarely only in the next days because of holidays. But I'm happy to answer any questions about metricbeat in more detail here if needed.

This comment has been minimized.

Copy link
@scottcrespo

scottcrespo Dec 29, 2016

Author Contributor

@ruflin

No problem! I've been on vacation as well, but I have a few days to work on beats before taking some additional time off.

I'm in the process of updating the mongodb module to establish direct connections to the hosts listed in the config, and to persist those connections between calls. I should be able to update this PR within a few days!

Scott Crespo
Scott Crespo added 20 commits Dec 22, 2016
Scott Crespo
Scott Crespo
changed database response back to map[string]interface{}{}. otherwise…
… fields get lost when calling eventMapping()
Scott Crespo
debugged integration tests to work with metricbeats testsuite. added …
…additional fields to dbstats metricset
Scott Crespo
Merge pull request #1 from scottcrespo/metricbeat-mongodb-persistent-…
…connections

Metricbeat mongodb persistent connections
@scottcrespo

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2016

@ruflin

Ready for your review. Please let me know what changes I might need to make!

Scott Crespo

@ruflin ruflin added review and removed in progress labels Jan 2, 2017

@ruflin
Copy link
Collaborator

left a comment

Thanks for pushing this forward. Also finally back online again.

In general changes LGTM. I left some minor comments / question. The major logical change is that now Mongodb connects to a specific host instead of the cluster, correct?

@tsg Could you also have a look at the changes as you did the mongo implementation as far as I remember.

@@ -117,7 +117,7 @@ metricbeat.modules:

#------------------------------- MongoDB Module ------------------------------
#- module: mongodb
#metricsets: ["status"]
#metricsets: ["status", "dbstats"]

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

We should probably sort this alphabetically. But not sure if we are consistent in other modules with that ...

},
"mongodb":{
"dbstats":{
"example": "dbstats"

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

An example doc should be generated here with make generate-json. But that can still be done after merging the PR.

- name: objects
type: long

- name: storage_size

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

what is the unit for all the _size fields?

- name: file_size
type: long

- name: index_size

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

this is probably index_size.bytes and then you can add format: bytes to each block, so Kibana will automatically now that it is in bytes and can do some conversion.

"indexes": c.Int("indexes"),
"index_size": c.Int("indexSize"),
// mmapv1 only
"ns_size_mb": c.Int("nsSizeMB"),

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

Should this field be optional as based on the comment, I assume it does not always exist?

// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
dialInfo, err := mgo.ParseURL(base.HostData().URI)

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

Could you add an experimental flag here? We normally introduce new metricsets as experimental. This allows us to first get some real word feedback for it and still change the data structure if needed without having to wait for a major release. Have a look here: https://github.com/elastic/beats/blob/master/metricbeat/module/haproxy/stat/stat.go#L34

nodeDialInfo.FailFast = true

logp.Info("Connecting to MongoDB node at %v", nodeDialInfo.Addrs)
fmt.Printf("Connecting to MongoDB node at %v", nodeDialInfo.Addrs)

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

The fmt should be removed here as we already log it.

if err != nil {
return nil, err
}

return &MetricSet{
BaseMetricSet: base,
dialInfo: dialInfo,

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

Do we still need to store the dialInfo as we use now mongoSession?

}
defer session.Close()

session.SetMode(mgo.Monotonic, true)

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

the monotonic mode was removed. Is this intentional?

This comment has been minimized.

Copy link
@scottcrespo

scottcrespo Jan 8, 2017

Author Contributor

@ruflin

Yes, this was intentional. Monotonic Mode connects to the cluster as a whole and reads from the nearest secondary (not clear how nearest is defined). So, if your desired target is busy or goes down, mgo will read from a different node in the cluster. This failover strategy is what I was referring to earlier (which I think is undesirable) - where you may read from a node other than the target specified in the module's configuration, and obfuscate node failures.

Thus, the change I've implemented establishes direct connections to each of the nodes specified in the module's settings. You can see how I did it here. This ensures that we are reading from the intended node. And, if it's slow or fails, we will know about it.

Also, note that I've also set FailFast to True, which helps identity latent and non-responsive servers sooner rather than later.

@@ -17,43 +15,52 @@ TODOs:
* add a metricset for "metrics" data
*/

var debugf = logp.MakeDebug("mongodb.status")

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 3, 2017

Collaborator

I assume you removed this, because it was not used. We normally keep this in to make it easy to add debug messages.

@scottcrespo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2017

@ruflin

Thanks for all the feedback! I'm offline this week, but I'll implement your suggestions startkng next week!

@scottcrespo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2017

@ruflin

Implemented all of your suggestions. Ready for you to take another look =)

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2017

jenkins, test it

@ruflin
ruflin approved these changes Jan 10, 2017
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2017

@scottcrespo LGTM. Could you add an update the CHANGELOG.asciidoc and squash the commits into 1?
@tsg Could you also have a quick look?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2017

jenkins, test it

@ruflin ruflin merged commit 2ddb800 into elastic:master Jan 11, 2017

4 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2017

@scottcrespo Merged. Thanks a lot for this contribution.

@scottcrespo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

@ruflin

You're welcome! It's been great working with you on this. Thanks for all of the help and feedback.

@scottcrespo scottcrespo deleted the scottcrespo:mongo-dbstats branch Jan 11, 2017

martinscholz83 added a commit to martinscholz83/beats that referenced this pull request Jan 16, 2017
Add Mongo dbstats metricset to Metricbeat (elastic#3228)
* set FailFast = True for direct connections to prevent a nonresponsive node from holding up the reporting
* new implementation that establishes direct connetions to mongo hosts
* successful reporting using direct node connections
* implementing multi-node direct reporting for mongodb.serverStatus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.