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

Add Prom/Grafana dashboard for M3DB as well as docker-compose for local development of m3-stack #939

Merged
merged 26 commits into from
Sep 25, 2018

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Sep 25, 2018

  • JSON for M3DB Grafana Dashboard
  • M3 stack "in a box", one line command to: start 3 m3db nodes (including namespace/topology initializing), an m3coordinator node, a prometheus node, and a grafana node with scraping and remote read/write setup. Grafana prometheus source and M3DB dashboard also auto-populate.

github_token Outdated
@@ -0,0 +1 @@
f6192b8d35518098fbb645ffd807a53749ffb00e
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this should be here, might be a good idea to regenerate your tokens too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I deleted the access token, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was actually deleted before I accidentally pushed it here, but I'll reach out to GitHub support as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK just verified the access token doesn't work by running:

curl -H "Authorization: token f6192b8d35518098fbb645ffd807a53749ffb00e" https://api.github.com

which returned

{
  "message": "Bad credentials",
  "documentation_url": "https://developer.github.com/v3"
}

So I must have deleted the token awhile back before it ever made it into this P.R

We're good 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just make sure you delete the token in GH settings and should be good

@@ -281,15 +281,15 @@ func newSession(opts Options) (clientSession, error) {
s.pools.writeAttempt.Init()

fetchAttemptPoolOpts := pool.NewObjectPoolOptions().
SetSize(opts.FetchBatchOpPoolSize()).
SetSize(1).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unintentional

@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #939 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
- Coverage   77.96%   77.92%   -0.04%     
==========================================
  Files         411      411              
  Lines       34406    34406              
==========================================
- Hits        26824    26812      -12     
- Misses       5739     5749      +10     
- Partials     1843     1845       +2
Flag Coverage Δ
#dbnode 81.48% <ø> (-0.05%) ⬇️
#m3ninx 75.21% <ø> (ø) ⬆️
#query 64.3% <ø> (-0.02%) ⬇️
#x 84.72% <ø> (ø) ⬆️

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 eef049a...edd73a8. Read the comment docs.

github_token Outdated
@@ -0,0 +1 @@
f6192b8d35518098fbb645ffd807a53749ffb00e
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanna delete this file now?


This docker-compose file will setup the following environment:

1. 3 M3DB nodes with a single node acting as an EtcD seed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ETCD

.gitignore Outdated
@@ -46,3 +46,6 @@ site/
!m3db.io/**/vendor
# Automatically populated from asset sources
m3db.io/openapi

# GitHub API token
github_token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need this change once you delete the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where goreleaser expects it to be so might save someone else the trouble

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh separate PRs mate =P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fiiiiiine haha

@@ -0,0 +1,212 @@
coordinator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: dbnode_config.yaml and coordinator config here - i'm a little concerned about the duplication of configs - it's one more thing to keep in sync. Anyway we can merge with others we already have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already sync'd with @robskillington and this is the best we can do for now until we can get some kind of templating system, I already did some kind of weird stuff to avoid having 3 different m3db configs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh

@richardartoul richardartoul changed the title [WIP] Add Prom/Grafana dashboard for M3DB Add Prom/Grafana dashboard for M3DB as well as docker-compose for local development of m3-stack Sep 25, 2018
@@ -9,7 +9,7 @@ PARAM_TEST_BUILD="${TEST_BUILD:-true}"
PARAM_TEST_VERIFY="${TEST_VERIFY:-true}"
PARAM_TEST_TEARDOWN="${TEST_TEARDOWN:-true}"

if [ $PARAM_TEST_BUILD != "true" ]; then
if [ "$PARAM_TEST_BUILD" != "true" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rewire this test to just use your new m3_stack stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also synced with @robskillington and want to keep them separate to make things easier, but also so the default configuration for the local stack can be a lot heavier

}
}
}'
echo "Done initializing namespace"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: steal the validation that paul does in the integration test - i.e. the curl ... | jq ... bits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"intervalFactor": 1,
"key": 0.08874521907599053,
"refId": "A",
"target": "fetch service:statsdex_m3dbnode name:database.bootstrapped host:$servers | <1 | removeEmpty | count"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove all target etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"renderer": "flot",
"seriesOverrides": [
{
"alias": "divideSeries(sumSeries(perSecond(statsdex.metrics.query.mquery01-sjc1.storage.m3dbshadow.mismatches)),sumSeries(perSecond(statsdex.metrics.query.mquery01-sjc1.storage.m3dbshadow.matches)))",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shit too

"renderer": "flot",
"seriesOverrides": [
{
"alias": "divideSeries(sumSeries(perSecond(statsdex.metrics.query.mquery01-sjc1.storage.m3dbshadow.mismatches)),sumSeries(perSecond(statsdex.metrics.query.mquery01-sjc1.storage.m3dbshadow.matches)))",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just checking prom expressions, lgtm, but may need to run the configs by the others

"tableColumn": "Value",
"targets": [
{
"expr": "sum(database_bootstrapped{instance=~\"$instance\"} == 1)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... == bool 1) may be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call

"steppedLine": false,
"targets": [
{
"expr": "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably delete this, unless it's for formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a "not implemented" graph

"sort": 0,
"value_type": "cumulative"
},
"type":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can get rid of this unless this is some formatting stuff it adds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment


## Usage

Use the `start.sh` and `stop.sh` scripts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe give these scripts more descriptive names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

COPY ./docker/grafana/dashboards.yaml /etc/grafana/provisioning/dashboards/all.yaml
COPY ./integrations/grafana/m3db_dashboard.json /var/lib/grafana/dashboards/m3db_dashboard.json
# Need to replace datasource template variable with name of actual data source so auto-import
# JustWorksTM. Need to switch users because the /var/lib/grafana/dashboards directory is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@@ -0,0 +1,28 @@
# Local Development
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind linking a new section in https://github.com/m3db/m3/blob/master/DEVELOPER.md to this page

ports:
- "0.0.0.0:7201:7201"
- "0.0.0.0:7203:7203"
- "0.0.0.0:7208:7208"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

expose:
- "7201"
- "7203"
- "7208"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Collaborator

@prateek prateek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ last couple of nits

@richardartoul richardartoul merged commit 6aa47dd into master Sep 25, 2018
@prateek prateek deleted the ra/prom branch September 29, 2018 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants