-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
docs: Added MongoDB sink RFC #3681
Conversation
Signed-off-by: James Turnbull <james@lovedthanlost.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good! Mostly just some comments about the metric naming.
I am curious how we'll test this one. I guess we could do an integration test with a mongodb container that doesn't bother asserting the values, but just the scraped metric names.
Apologies for the delay in review! This one had fallen off my radar. |
Signed-off-by: James Turnbull <james@lovedthanlost.net>
Signed-off-by: James Turnbull <james@lovedthanlost.net>
Signed-off-by: James Turnbull <james@lovedthanlost.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I still didn't dig too deeply into the stats themselves having little familiarity with MongoDB.
- `mongodb_extra_info_heap_usage_bytes` (gauge) | ||
- `mongodb_extra_info_page_faults` (gauge) | ||
- `mongodb_instance_local_time` (gauge) | ||
- `mongodb_instance_uptime_estimate_seconds_total` (counter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not clear to me what the difference is between this one and mongodb_instance_uptime_seconds_total
; even after looking at https://docs.mongodb.com/manual/reference/command/serverStatus/ . Are you aware? The number seems to be the same when I ran a local mongodb docker container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually can't work out the difference.
uptime
The number of seconds that the current MongoDB process has been active.
uptimeEstimate
The uptime in seconds as calculated from MongoDB’s internal course-grained time keeping system.
I guess someone might want both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely unclear to me 😄 but I'm fine with reporting both given that serverStatus
does. Maybe they differ in certain circumstances.
Signed-off-by: James Turnbull <james@lovedthanlost.net>
Ping @JeanMertz @binarylogic I need two more approvals to merge this. Thanks! |
Signed-off-by: James Turnbull <james@lovedthanlost.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice work. I have no idea what these metrics mean, but I'm happy to see they are consistent with the Prometheus metrics. I do wonder if the naming could be improved though. Prefixes like mongodb_mongod_metrics
seem redundant, but I don't have enough MongoDB context to know for sure.
Something like |
Last call @JeanMertz :) Otherwise, I'm going to accept the two approvals here - if others have feedback feel free to reach out prior to implementation. |
* draft Signed-off-by: James Turnbull <james@lovedthanlost.net> * More edits Signed-off-by: James Turnbull <james@lovedthanlost.net> * Addressed feedback from Jesse Signed-off-by: James Turnbull <james@lovedthanlost.net> * Fixed checks Signed-off-by: James Turnbull <james@lovedthanlost.net> * Fixed two more metric names Signed-off-by: James Turnbull <james@lovedthanlost.net> * More edits from Jesse Signed-off-by: James Turnbull <james@lovedthanlost.net> * Added full metrics Signed-off-by: James Turnbull <james@lovedthanlost.net> Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Closes #3641