Add namespace and placement info to debug endpoint in coordinator #1896

benraskin92 · 2019-08-20T18:29:43Z

What this PR does / why we need it:

This PR adds namespace and placement info the /debug/dump endpoint in coordinator.

Add tests to namespace.go
Add placement info
Add documentation for debug endpoint (if there isn't already)

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

This PR adds placement and namespace information to the `/debug/dump` api endpoint within coordinator.

Does this PR require updating code package or user-facing documentation?:

Will add documentation around `/debug/dump` API endpoint in a separate PR.

benraskin92 · 2019-08-20T18:30:10Z

scripts/development/m3_stack/start_m3.sh

@@ -26,7 +26,7 @@ if [[ "$USE_JAEGER" = true ]] ; then
    fi
 fi

-docker-compose -f docker-compose.yml up $DOCKER_ARGS m3coordinator01
+docker-compose -f docker-compose.yml up --build -d --renew-anon-volumes m3coordinator01


Remove before landing.

codecov · 2019-08-21T16:04:17Z

Codecov Report

Merging #1896 into master will decrease coverage by 4%.
The diff coverage is 0.9%.

@@            Coverage Diff            @@
##           master   #1896      +/-   ##
=========================================
- Coverage    61.5%   57.4%    -4.1%     
=========================================
  Files        1100    1039      -61     
  Lines      104214   91971   -12243     
=========================================
- Hits        64136   52873   -11263     
+ Misses      35843   35230     -613     
+ Partials     4235    3868     -367

Flag	Coverage Δ
#aggregator	`76% <ø> (-3.8%)`	⬇️
#cluster	`52.1% <ø> (-4.2%)`	⬇️
#collector	`54.9% <ø> (-8.8%)`	⬇️
#dbnode	`68.4% <0%> (+5.4%)`	⬆️
#m3em	`41.2% <ø> (-5.3%)`	⬇️
#m3ninx	`55.1% <ø> (-6%)`	⬇️
#m3nsch	`100% <ø> (+48.8%)`	⬆️
#metrics	`17.5% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (+0.8%)`	⬆️
#query	`40.1% <0%> (-28.1%)`	⬇️
#x	`64.7% <1.6%> (+3.2%)`	⬆️

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 94cadd4...1214da8. Read the comment docs.

arnikola · 2019-08-21T17:21:14Z

src/query/server/server.go

+		logger.Fatal("unable to get placement opts", zap.Error(err))
+	}
+
+	debugWriter, err := xdebug.NewZipWriterWithDefaultSources(


This can be done inside the if block; doesn't seem to be used otherwise

arnikola · 2019-08-21T17:22:48Z

src/query/server/server.go

+			mux := http.NewServeMux()
+			if debugWriter != nil {
+				if err := debugWriter.RegisterHandler(debugEndpoint, mux); err != nil {
+					logger.Error("unable to register debug writer endpoint", zap.Error(err))


nit: print the config listen address here?

arnikola · 2019-08-21T17:27:02Z

src/cmd/services/m3query/config/config.go

@@ -94,6 +94,9 @@ type Configuration struct {
 	// ListenAddress is the server listen address.
 	ListenAddress *listenaddress.Configuration `yaml:"listenAddress" validate:"nonzero"`

+	// The host and port on which to listen for debug endpoints.
+	DebugListenAddress string `yaml:"debugListenAddress"`


Might be a good idea to get into the habit of editing annotated config when we add config settings (think it's quite behind unfortunately)

Yeah I'm working on restructuring the docs so lemme jot this down.

arnikola · 2019-08-21T17:29:31Z

src/query/api/v1/httpd/handler.go

@@ -293,6 +289,20 @@ func (h *Handler) RegisterRoutes() error {
 	return nil
 }

+// PlacementOpts returns placement opts used in the various placement APIs.
+func (h *Handler) PlacementOpts() (placement.HandlerOptions, error) {
+	placementOpts, err := placement.NewHandlerOptions(


nit: return placement.NewHandlerOptions(..)

arnikola · 2019-08-21T17:29:53Z

src/query/api/v1/httpd/handler.go

@@ -293,6 +289,20 @@ func (h *Handler) RegisterRoutes() error {
 	return nil
 }

+// PlacementOpts returns placement opts used in the various placement APIs.


nit: placement options in comment

nerd0 · 2019-08-21T17:38:42Z

src/query/api/v1/httpd/handler.go

 		if err != nil {
-			return err
+			return nil


is nil suppose to be returned? If so can you drop a comment about why?

arnikola · 2019-08-21T17:34:30Z

src/query/server/server.go

+					zap.String("address", cfg.DebugListenAddress), zap.Error(err))
+			} else {
+				logger.Info("debug server listening",
+					zap.String("address", cfg.DebugListenAddress),


Nit: might be better to also include the endpoint, i.e. zap.String("endpoint", fmt.Sprintf("%s%s", cfg.DebugListenAddress, debugEndpoint))

src/x/debug/debug.go

arnikola · 2019-08-21T17:41:46Z

src/x/debug/namespace.go

+	getHandler *namespace.GetHandler
+}
+
+// NewNamespaceInfoSource returns a Source for namespace information


nit; . at the end of comments

arnikola · 2019-08-21T17:42:12Z

src/x/debug/namespace.go

+)
+
+type namespaceInfoSource struct {
+	getHandler *namespace.GetHandler


nit: handler rather than getHandler

robskillington · 2019-08-23T21:01:58Z

scripts/development/m3_stack/m3coordinator.yml

@@ -2,6 +2,8 @@ listenAddress:
  type: "config"
  value: "0.0.0.0:7201"

+debugListenAddress: "0.0.0.0:7205"


Maybe we put this on the main listen address on 7201?

robskillington · 2019-08-23T21:21:44Z

src/x/debug/namespace.go

+	}
+
+	w.Write(jsonData)
+	return nil


This should probably be return w.Write(jsonData) since Write(...) can return an error

robskillington · 2019-08-23T21:21:54Z

src/x/debug/namespace.go

+
+	marshaler := jsonpb.Marshaler{EmitDefaults: true}
+	buf := new(bytes.Buffer)
+	marshaler.Marshal(buf, resp)


Need to check if this returns an error yeah?

robskillington · 2019-08-23T21:22:31Z

src/x/debug/placement.go

+	marshaler := jsonpb.Marshaler{EmitDefaults: true}
+	marshaler.Marshal(w, resp)
+
+	return nil


Similarly, here need to check marshaler.Marshal(...) so maybe just return marshaler.Marshal(w, resp)

robskillington · 2019-08-23T21:23:05Z

src/x/debug/placement.go

+// Write fetches data about the placement and writes it in the given writer.
+// The data is formatted in json.
+func (p *placementInfoSource) Write(w io.Writer) error {
+	placement, _, err := p.getHandler.Get(defaultM3DBServiceName, nil)


Maybe we should make the service name be part of the constructor so it can be reused? i.e. NewPlacementInfoSource(serviceName string, ...)

Also maybe make NewPlacementZipWriterWithDefaultSources(...) take a serviceNames []string param and create a new placement info source per service names specified?

robskillington · 2019-08-23T21:23:56Z

src/x/debug/debug.go

@@ -68,9 +71,44 @@ func NewZipWriter(iopts instrument.Options) ZipWriter {
 	}
 }

+// NewPlacementZipWriterWithDefaultSources returns a zipWriter with the following
+// debug sources already registered: CPU, heap, host, goroutines, namespace and placement info.
+func NewPlacementZipWriterWithDefaultSources(


nit: NewPlacementAndNamespaceZipWriterWithDefaultSources(...) so it mentions namespace?

benraskin92 · 2019-08-26T01:48:34Z

src/query/api/v1/httpd/handler.go

+
+		// Register debug dump handler.
+		h.router.HandleFunc(xdebug.DebugURL,
+			wrapped(debugWriter.HTTPHandler()).ServeHTTP)


@robskillington Do I need to include .Methods() on this?

benraskin92 · 2019-08-26T01:57:18Z

src/query/api/v1/httpd/handler.go

+	queryContextOptions   models.QueryContextOptions
+	instrumentOpts        instrument.Options
+	cpuProfileDuration    time.Duration
+	placementServiceNames []string


Could also create a debugOpts struct?

robskillington

LGTM other than one nit

robskillington · 2019-08-26T02:58:33Z

src/x/debug/debug.go

 	"go.uber.org/zap"
 )

+const (
+	DebugURL = "/debug/dump"


Not: need to add comment for exported var

benraskin92 commented Aug 20, 2019

View reviewed changes

benraskin92 requested a review from robskillington August 21, 2019 15:05

benraskin92 force-pushed the braskin/m3db_debug_setup branch from be6b355 to 692b3c2 Compare August 21, 2019 16:02

benraskin92 changed the title ~~[DRAFT] Add namespace info to debug endpoint~~ Add namespace and placement info to debug endpoint in coordinator Aug 21, 2019

benraskin92 requested review from richardartoul and arnikola August 21, 2019 16:04

arnikola reviewed Aug 21, 2019

View reviewed changes

nerd0 reviewed Aug 21, 2019

View reviewed changes

arnikola reviewed Aug 21, 2019

View reviewed changes

benraskin92 force-pushed the braskin/m3db_debug_setup branch from 9af18f7 to aa38e73 Compare August 22, 2019 16:12

robskillington reviewed Aug 23, 2019

View reviewed changes

benraskin92 commented Aug 26, 2019

View reviewed changes

robskillington approved these changes Aug 26, 2019

View reviewed changes

src/x/debug/debug.go

"go.uber.org/zap"

)

const (

DebugURL = "/debug/dump"

Copy link

Collaborator

robskillington Aug 26, 2019

Choose a reason for hiding this comment

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

Not: need to add comment for exported var

benraskin92 added 6 commits August 25, 2019 23:03

Add namespace info to debug endpoint

3415788

Update tests

f576e74

Change mux server

6edf10c

Add tests to ns and placement source

3afd582

Address comments

3f8b76a

Address more comments

1f03a8f

benraskin92 added 8 commits August 25, 2019 23:03

Naming

0e0f3a1

Comments

8709e15

Comments

483bd11

Comments

c8ec620

Address more comments

fe628b7

Remove separate debug listen address

4cf929a

Revert start_m3

a622f0c

Add comment and fix test

1214da8

benraskin92 force-pushed the braskin/m3db_debug_setup branch from 2076fdd to 1214da8 Compare August 26, 2019 03:04

Fix comment

3335303

benraskin92 merged commit 8ad9b8e into master Aug 26, 2019

benraskin92 deleted the braskin/m3db_debug_setup branch August 26, 2019 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace and placement info to debug endpoint in coordinator #1896

Add namespace and placement info to debug endpoint in coordinator #1896

benraskin92 commented Aug 20, 2019 •

edited

Loading

benraskin92 Aug 20, 2019

arnikola Aug 21, 2019

codecov bot commented Aug 21, 2019 •

edited

Loading

arnikola Aug 21, 2019

arnikola Aug 21, 2019

arnikola Aug 21, 2019

benraskin92 Aug 21, 2019

arnikola Aug 21, 2019

arnikola Aug 21, 2019

nerd0 Aug 21, 2019

arnikola Aug 21, 2019

arnikola Aug 21, 2019

arnikola Aug 21, 2019

robskillington Aug 23, 2019

robskillington Aug 23, 2019

robskillington Aug 23, 2019

robskillington Aug 23, 2019

robskillington Aug 23, 2019 •

edited

Loading

robskillington Aug 23, 2019

robskillington Aug 23, 2019

benraskin92 Aug 26, 2019

benraskin92 Aug 26, 2019

robskillington left a comment

robskillington Aug 26, 2019

Add namespace and placement info to debug endpoint in coordinator #1896

Add namespace and placement info to debug endpoint in coordinator #1896

Conversation

benraskin92 commented Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benraskin92 commented Aug 20, 2019 •

edited

Loading

codecov bot commented Aug 21, 2019 •

edited

Loading

robskillington Aug 23, 2019 •

edited

Loading