[query]Add debug param to namespace get API #1698

benraskin92 · 2019-06-03T20:44:07Z

What this PR does / why we need it:

This PR allows you to pass in an optional debug parameter to the /api1/v1/namespace endpoint in the coordinator, which returns durations instead of nanoseconds for the duration fields. This should help with readability and debugging. Below is a sample output using start_m3.

{
  "registry": {
    "namespaces": {
      "metrics_0_30m": {
        "bootstrapEnabled": true,
        "cleanupEnabled": true,
        "flushEnabled": true,
        "indexOptions": {
          "blockSizeDuration": "10m0s",
          "enabled": true
        },
        "retentionOptions": {
          "blockDataExpiry": true,
          "blockDataExpiryAfterNotAccessPeriodDuration": "5m0s",
          "blockSizeDuration": "10m0s",
          "bufferFutureDuration": "5m0s",
          "bufferPastDuration": "5m0s",
          "retentionPeriodDuration": "30m0s"
        },
        "snapshotEnabled": true,
        "writesToCommitLog": true
      },
      "metrics_10s_48h": {
        "bootstrapEnabled": true,
        "cleanupEnabled": true,
        "flushEnabled": true,
        "indexOptions": {
          "blockSizeDuration": "4h0m0s",
          "enabled": true
        },
        "retentionOptions": {
          "blockDataExpiry": true,
          "blockDataExpiryAfterNotAccessPeriodDuration": "5m0s",
          "blockSizeDuration": "4h0m0s",
          "bufferFutureDuration": "10m0s",
          "bufferPastDuration": "10m0s",
          "retentionPeriodDuration": "48h0m0s"
        },
        "snapshotEnabled": true,
        "writesToCommitLog": true
      }
    }
  }
}

Special notes for your reviewer:

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

Users can now pass in a `debug` parameter when making a call to the `/api/v1/namespace` endpoint in coordinator. When set to `true` it allows for better readability.

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

Added docs to the `namespace_configuration.md` file under the operational guides.

mway · 2019-06-04T00:52:30Z

src/query/api/v1/handler/namespace/get.go

@@ -71,6 +74,19 @@ func (h *GetHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		Registry: &nsRegistry,
 	}

+	urlParams := r.URL.Query()
+	if strings.ToLower(urlParams.Get("debug")) == "true" {


let's do

if debug, err := strconv.ParseBool(r.URL.Query().Get(_debugParam)); err == nil && debug { // ... xhttp.WriteJSONResponse(w, nanosToDurationMap, logger) return }

(consting _debugParam as well)

src/query/api/v1/handler/namespace/get.go

mway · 2019-06-04T01:09:42Z

src/query/api/v1/handler/namespace/get.go

+
+func nanosToDuration(resp proto.Message) (map[string]interface{}, error) {
+	marshaler := jsonpb.Marshaler{EmitDefaults: true}
+	nsString, err := marshaler.MarshalToString(resp)


Can we just use an io.ReadWriter here to skip the intermediate string?

mway · 2019-06-04T01:11:17Z

src/x/net/http/convert.go

+func NanosToDurationBytes(r io.Reader) (map[string]interface{}, error) {
+	var dict map[string]interface{}
+	d := json.NewDecoder(r)
+	d.UseNumber()


You're already handling the float64 case below, wouldn't it be simpler just to stick with that?

So I think we actually want something like this {"fieldNanos":100.5} to fail (e.g. nanos shouldn't be floats). I'm fine with removing the case float64 below though as it will never actually be called.

src/x/net/http/convert.go

richardartoul

@benraskin92 The namespace API accepts input in the format of durations right? Can we just drop the nanos entirely and make the debug=true behavior the default behavior?

I can't think of a single situation where I'd ever want to interact with raw nanos here

codecov · 2019-06-04T15:29:30Z

Codecov Report

Merging #1698 into master will decrease coverage by 17.2%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #1698      +/-   ##
=========================================
- Coverage    71.9%   54.6%   -17.3%     
=========================================
  Files         970     859     -111     
  Lines       81324   73103    -8221     
=========================================
- Hits        58476   39964   -18512     
- Misses      19016   29948   +10932     
+ Partials     3832    3191     -641

Flag	Coverage Δ
#aggregator	`74.1% <ø> (-8.3%)`	⬇️
#cluster	`51.8% <ø> (-34%)`	⬇️
#collector	`48% <ø> (-15.9%)`	⬇️
#dbnode	`64.3% <ø> (-15.6%)`	⬇️
#m3em	`49.6% <ø> (-23.7%)`	⬇️
#m3ninx	`54.6% <ø> (-19.5%)`	⬇️
#m3nsch	`100% <ø> (+48.8%)`	⬆️
#metrics	`?`
#msg	`74.5% <ø> (-0.3%)`	⬇️
#query	`25.5% <0%> (-40.9%)`	⬇️
#x	`56.3% <0%> (-29.6%)`	⬇️

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 c0d102f...237722b. Read the comment docs.

benraskin92 · 2019-06-04T15:31:00Z

@benraskin92 The namespace API accepts input in the format of durations right? Can we just drop the nanos entirely and make the debug=true behavior the default behavior?

I can't think of a single situation where I'd ever want to interact with raw nanos here

If we do that, I'd rather do that in another PR. I'm all for making it the default though.

schallert · 2019-06-05T15:50:25Z

@benraskin92 The namespace API accepts input in the format of durations right? Can we just drop the nanos entirely and make the debug=true behavior the default behavior?
I can't think of a single situation where I'd ever want to interact with raw nanos here

If we do that, I'd rather do that in another PR. I'm all for making it the default though.

I don't think we can do that without adding a new endpoint or some careful coordinator with operator compatibility. The operator parses the responses from the coordinator as jsonpb Proto messages, changing the proto or the response marshaling would likely break the operator.

If we really want to make that change we have to do it in a manner such that users of older operator with newer M3DB and vice-versa don't have breakages.

prateek · 2019-06-05T15:55:44Z

+1 to Matt’s point. We should stick with current format for b/w compat.

More generally, when we add a separate debug endpoint (should do this for a variety of reasons), we can ensure it calls the format that’s easier to grok.

prateek

LGTM

richardartoul · 2019-06-05T16:24:18Z

@schallert @prateek Fair enough. I assumed this only affected the JSON output and that the operator was using GRPC or something but that makes sense

Benjamin Raskin added 3 commits June 3, 2019 16:43

Add debug param to namespace get api

d43cc6c

Fix test

4dd0710

Add docs

7699890

benraskin92 requested review from richardartoul, justinjc and robskillington June 3, 2019 23:35

benraskin92 changed the title ~~[WIP][NO REVIEW][query]Add debug param to namespace get api~~ [query]Add debug param to namespace get API Jun 3, 2019

mway reviewed Jun 4, 2019

View reviewed changes

richardartoul reviewed Jun 4, 2019

View reviewed changes

Address comments

237722b

prateek approved these changes Jun 5, 2019

View reviewed changes

Merge branch 'master' into braskin/get_namespace_units

235d9fe

benraskin92 merged commit 0d57cfa into master Jun 5, 2019

benraskin92 deleted the braskin/get_namespace_units branch June 5, 2019 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query]Add debug param to namespace get API #1698

[query]Add debug param to namespace get API #1698

benraskin92 commented Jun 3, 2019 •

edited

Loading

mway Jun 4, 2019

mway Jun 4, 2019

mway Jun 4, 2019

benraskin92 Jun 4, 2019

richardartoul left a comment •

edited

Loading

codecov bot commented Jun 4, 2019

benraskin92 commented Jun 4, 2019

schallert commented Jun 5, 2019

prateek commented Jun 5, 2019

prateek left a comment

richardartoul commented Jun 5, 2019

[query]Add debug param to namespace get API #1698

[query]Add debug param to namespace get API #1698

Conversation

benraskin92 commented Jun 3, 2019 • edited Loading

mway Jun 4, 2019

Choose a reason for hiding this comment

mway Jun 4, 2019

Choose a reason for hiding this comment

mway Jun 4, 2019

Choose a reason for hiding this comment

benraskin92 Jun 4, 2019

Choose a reason for hiding this comment

richardartoul left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2019

Codecov Report

benraskin92 commented Jun 4, 2019

schallert commented Jun 5, 2019

prateek commented Jun 5, 2019

prateek left a comment

Choose a reason for hiding this comment

richardartoul commented Jun 5, 2019

benraskin92 commented Jun 3, 2019 •

edited

Loading

richardartoul left a comment •

edited

Loading