PMM-1764 Various small improvements #85

AlekSi · 2018-01-04T14:47:21Z

No description provided.

* Disable pre-fetching (not needed on exporter queries), limit conn pool size and re-use database connection to MongoDB * Remove .Ping() on exporter.mongoSess, mgo.Session will recreate the conn for us * avoid race condition with sync.Mutex * Use lock on close of mgo session also * Use lock on close of mgo session also #2 * make pool limit a flag instead of hard-code * default to 1 connection, it seems to work fine even at high volume * Must return .Copy() of mgo.Session to avoid mgo deadlocking under concurrent calls. Moved session SocketTimeout to no longer be forever * Must defer .Close() on copied mgo.Session from .getSession * must check if != nil a 2nd time in getSession()

* Use mutex hat. * Protect Close() with lock too. * Count getSession() errors.

* Added query comments to .Find() queries in oplog_status.go, created shared/utils.go function to add comments, cleaned up duplicated code I made long ago * shorten comment

codecov · 2018-01-04T14:58:06Z

Codecov Report

Merging #85 into develop will decrease coverage by 1%.
The diff coverage is 11.88%.

@@             Coverage Diff             @@
##           develop      #85      +/-   ##
===========================================
- Coverage    24.59%   23.58%   -1.01%     
===========================================
  Files           35       39       +4     
  Lines         1960     2226     +266     
===========================================
+ Hits           482      525      +43     
- Misses        1455     1678     +223     
  Partials        23       23

Impacted Files	Coverage Δ
collector/mongod/replset_status.go	`0% <ø> (ø)`	⬆️
collector/mongod/global_lock.go	`77.27% <ø> (ø)`	⬆️
collector/mongos/sharding_status.go	`0% <0%> (ø)`	⬆️
shared/utils.go	`0% <0%> (ø)`	⬆️
collector/mongod/collections_status.go	`0% <0%> (ø)`
collector/mongos/database_status.go	`0% <0%> (ø)`
mongodb_exporter.go	`1.53% <0%> (-0.05%)`	⬇️
collector/mongod/oplog_status.go	`0% <0%> (ø)`	⬆️
collector/mongos/collections_status.go	`0% <0%> (ø)`
collector/mongos/sharding_topology.go	`0% <0%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a65811a...f58598a. Read the comment docs.

arvenil · 2018-01-04T16:22:28Z

CHANGELOG.md

+  * `mongodb_up`
+* Some queries now contain [cursor comments](https://www.percona.com/blog/2017/06/21/tracing-mongodb-queries-to-code-with-cursor-comments/)
+  with source code locations.
 * Go vendoring switched to [dep](https://github.com/golang/dep).


I know this is not the part of this PR but when and why we switched to dep for this repo? Upstream still uses glide, this means harder merges from upstream, for what benefits?

We are on our own now. :)

arvenil · 2018-01-04T16:25:06Z

collector/mongodb_collector.go

+		return nil
+	}
+	return exporter.mongoSess.Copy()
+}


Sharing one MongoSession 👍
Wouldn't be easier though to create MongoSession once, when exporter starts? This way you will also avoid that Mutex and you can even produce one time warning/error if connection details are incorrect.

arvenil · 2018-01-04T16:33:10Z

collector/mongos/database_status.go

+		return nil
+	}
+	for _, db := range database_names {
+		if db == "admin" || db == "test" || db == "local" {


Why we skip those? Especially test?

arvenil · 2018-01-04T16:35:48Z

collector/mongodb_collector.go


-	mongoSess := shared.MongoSession(exporter.Opts.toSessionOps())
+	mongoSess := exporter.getSession()
 	if mongoSess == nil {


It works, error is logged in other place, however more clean way would be to bubble up the error instead of hiding it?

mongoSess, err := exporter.getSession() if err != nil { log.Error(err) }

I agree, but let's do it later.

arvenil · 2018-01-04T16:40:16Z

README.md

+```
+mongodb://CN=myName,OU=myOrgUnit,O=myOrg,L=myLocality,ST=myState,C=myCountry@localhost:27017/?authMechanism=MONGODB-X509
+```
+


Did we test that? I know it's mongodb_exporter repo, but did we put that also to our documentation for PMM? Link?

It's wasn't required.

arvenil · 2018-01-04T16:47:37Z

shared/utils.go

 }
+
+func AddCodeCommentToQuery(query *mgo.Query) *mgo.Query {
+	_, fileName, lineNum, ok := runtime.Caller(1)


This returns lineNum where AddCodeCommentToQuery was called? Could you add comment explaining that?

arvenil · 2018-01-04T17:10:16Z

shared/connection.go

+	session.SetPrefetch(0.00)
 	session.SetSyncTimeout(syncMongodbTimeout)
-	session.SetSocketTimeout(0)
+	session.SetSocketTimeout(dialMongodbTimeout)


This increases timeout to 3s?

0 meant "no timeout".

arvenil · 2018-01-04T17:13:28Z

shared/connection.go

+	// connect directly, fail faster, do not retry - for faster responses and accurate metrics, including mongoUp
+	dialInfo.Direct = true
 	dialInfo.Timeout = dialMongodbTimeout
+	dialInfo.FailFast = true


Makes sense, but why we then in other parts of the code retry on failed queries for tries < 2 {? (yeah, I know it's not a part of this PR, just a note)

Good point. We should remove them.

arvenil · 2018-01-04T17:25:38Z

shared/connection.go

 		return nil
 	}
 	session.SetMode(mgo.Eventual, true)
+	session.SetPoolLimit(opts.PoolLimit)


@timvaillancourt

Just a side note: https://github.com/go-mgo/mgo/issues/246#issuecomment-199907660

Also you could set pool limit in DSN with maxPoolSize:
https://github.com/go-mgo/mgo/blob/v2/session.go#L292-L296

While this certainly makes some sense, I'm not sure about performance gain? @AlekSi was this somehow verified? Would be nice to understand what was the real issue here, if it was not sharing session or lack of this pool limit (which seems weird, because the code is really linear so one request should use one socket).

arvenil · 2018-01-04T17:28:04Z

CHANGELOG.md


 ## v0.4.0 (not released yet)

+* New flags `-collect.database` and `-collect.collection` can be used to enable collection of database and collection


Was this somewhere requested? Jira issue?

That's #84.

timvaillancourt and others added 13 commits January 4, 2018 14:57

Minor refactoring.

b6b8e6e

* Use mutex hat. * Protect Close() with lock too. * Count getSession() errors.

Add MongoDB Query-comments to .Find() operations (#80)

f08e2e2

* Added query comments to .Find() queries in oplog_status.go, created shared/utils.go function to add comments, cleaned up duplicated code I made long ago * shorten comment

Tiny tweaks.

7c439c1

Document MONGODB-X509 auth

24a1cbc

adding database and collection stats collection

2e2b7e9

Unify flags style.

dfde402

Apply gofmt -s.

e08ab38

Remove unused metrics.

73540ad

Simplify boolean comparisons.

9a29c00

PMM-1586 Add «up» and «scrape_errors_total».

b912f8c

Update changelog.

7da4844

PMM-1586 Fix «up» metric to be accurate.

fbe1f0c

AlekSi requested a review from arvenil January 4, 2018 14:47

AlekSi mentioned this pull request Jan 4, 2018

adding database and collection stats collection #84

Closed

Test with upcoming version of PSMDB 3.6.

0cf5091

arvenil reviewed Jan 4, 2018

View reviewed changes

rnovikovP mentioned this pull request Jan 16, 2018

PMM-1587 Use mongodb_up variable for dashboards to identify if a host… percona/grafana-dashboards#89

Closed

AlekSi added 3 commits January 17, 2018 12:01

PMM-1764 Do not skip standard databases.

eaf2f65

PMM-1764 Add comment.

a804b8e

PMM-1764 Remove retries.

f58598a

AlekSi merged commit c1c7cff into develop Jan 17, 2018

AlekSi deleted the PMM-1764-small-improvements branch January 17, 2018 09:09


		## v0.4.0 (not released yet)

		* New flags `-collect.database` and `-collect.collection` can be used to enable collection of database and collection

PMM-1764 Various small improvements #85

PMM-1764 Various small improvements #85

Uh oh!

Conversation

AlekSi commented Jan 4, 2018

Uh oh!

codecov bot commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jan 4, 2018 •

edited

Loading