[dbnode] Validate indexable metric tags before insert into index queue #2046

robskillington · 2019-11-16T22:35:41Z

What this PR does / why we need it:

This also adds a tool to fix existing file sets.

Special notes for your reviewer:

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

NONE

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

NONE

arnikola · 2019-11-16T22:36:42Z

src/cmd/tools/verify_data_files/main/main.go

@@ -0,0 +1,398 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


codecov · 2019-11-16T22:43:14Z

Codecov Report

Merging #2046 into master will increase coverage by 9%.
The diff coverage is 51.1%.

@@           Coverage Diff            @@
##           master   #2046     +/-   ##
========================================
+ Coverage    63.3%   72.4%     +9%     
========================================
  Files        1001    1007      +6     
  Lines       86014   86414    +400     
========================================
+ Hits        54523   62571   +8048     
+ Misses      27580   19668   -7912     
- Partials     3911    4175    +264

Flag	Coverage Δ
#aggregator	`82% <ø> (+1.6%)`	⬆️
#cluster	`85.6% <ø> (+19%)`	⬆️
#collector	`64.8% <ø> (ø)`	⬆️
#dbnode	`79.7% <100%> (+3.9%)`	⬆️
#m3em	`73.2% <ø> (+0.9%)`	⬆️
#m3ninx	`73.9% <ø> (+2.9%)`	⬆️
#m3nsch	`51.1% <ø> (+22.7%)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (ø)`	⬆️
#query	`68.4% <50.5%> (+25.6%)`	⬆️
#x	`83.4% <ø> (+12%)`	⬆️

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 a112602...6a5832f. Read the comment docs.

arnikola · 2019-11-19T01:52:12Z

Makefile

@@ -89,6 +89,7 @@ TOOLS :=               \
 	clone_fileset        \
 	dtest                \
 	verify_commitlogs    \
+	verify_data_files   \


Does this need to be added to the license check? (Also comparator if you're adding that fix)

Added to FOSSA, looks like comparator is already there 👍

arnikola · 2019-11-19T03:02:53Z

src/dbnode/storage/index/convert/convert.go

+		return ErrUsingReservedFieldName
+	}
+	if !utf8.Valid(tagValue) {
+		return fmt.Errorf("document contains invalid field value: %v", tagValue)


nit; add name in error log

Sounds good

arnikola · 2019-11-19T03:04:14Z

src/dbnode/storage/index/convert/convert.go

+func ValidateMetricTag(tag ident.Tag) error {
+	tagName := tag.Name.Bytes()
+	tagValue := tag.Value.Bytes()
+	if !utf8.Valid(tagName) {


nit: Is this a more likely error scenario than the ReservedFieldNameID case? Also can probably wait to get the value bytes until after name passes validation

arnikola · 2019-11-19T03:36:04Z

src/cmd/tools/verify_data_files/main/main.go

+					"name_as_string=%s, name_as_hex=%s"+
+					"value_as_string=%s, value_as_hex=%s",
+					err,
+					tag.Name.Bytes(), tag.Name.Bytes(),


I think first one of these should be .String()

Whether you give it a string or []byte it acts the same thankfully - avoids allocations if leave it as []byte so I usually don't provide as a string to the formatter. e.g.:
https://play.golang.org/p/-yhIOkLan9H

arnikola · 2019-11-19T03:42:12Z

src/cmd/tools/verify_data_files/main/main.go

+	if !utf8.Valid(id.Bytes()) {
+		return readEntryResult{invalidID: true},
+			fmt.Errorf("invalid id: err=%s, as_string=%s, as_hex=%x",
+				"non-utf8", id.Bytes(), id.Bytes())


I think first should be a string

Whether you give it a string or []byte it acts the same thankfully - avoids allocations if leave it as []byte so I usually don't provide as a string to the formatter. e.g.:
https://play.golang.org/p/-yhIOkLan9H

arnikola · 2020-01-04T23:07:06Z

src/dbnode/storage/index/convert/convert.go

+// ValidateSeriesTag validates a series tag for use with m3ninx.
+func ValidateSeriesTag(tag ident.Tag) error {
+	tagName := tag.Name.Bytes()
+	tagValue := tag.Value.Bytes()


nit: May be better to only take the value after name validation passes?

arnikola · 2020-01-04T23:18:27Z

src/x/ident/types.go

@@ -241,6 +241,11 @@ func NewTags(values ...Tag) Tags {
 	return Tags{values: values}
 }

+// Reset resets the tags for reuse.
+func (t *Tags) Reset(values []Tag) {
+	t.values = values


Does this need to take ownership of the tags?

Followed up, ident.ID does the ref tracking so we are ok here.

arnikola · 2020-01-04T23:26:56Z

src/cmd/tools/verify_data_files/main/main.go

+	}
+
+	// Get all fileset files.
+	log.Info("discovering file sets",


meganit: could build up a fields := []zapcore.Fields{zap.Strings("namespaces", namespaces)} and append to it as we go?

arnikola · 2020-01-04T23:31:50Z

src/cmd/tools/verify_data_files/main/main.go

+		return nil, fmt.Errorf("could not read dir names: %v", err)
+	}
+
+	results := make([]string, 0, len(entries))


I think you can do this in place without an alloc results := entries[:0] and keep the rest the same?

Sounds good.

arnikola · 2020-01-04T23:33:14Z

src/cmd/tools/verify_data_files/main/main.go

+
+	results := make([]string, 0, len(entries))
+	for _, p := range entries {
+		if p == "." || p == ".." || p == "./.." || p == "./" || p == "../" || p == "./../" {


can you do if p[0] == '.' { continue } ? Or is there a chance there'll be valid files starting with .?

Correct they could be "./foo"

arnikola · 2020-01-04T23:39:02Z

src/cmd/tools/verify_data_files/main/main.go

+		return err
+	}
+
+	defer reader.Close()


Should this be deferred earlier before the error above?

Close() is only valid after an Open(), hence doing it below the return

arnikola · 2020-01-04T23:42:55Z

src/cmd/tools/verify_data_files/main/main.go

+
+		check, err := readEntry(id, tags, data, checksum)
+		data.Finalize() // Always finalize data.
+		if err != nil {


nit: reduce nesting with a continue?

if err == nil { continue }

Sounds good.

arnikola · 2020-01-04T23:58:39Z

src/cmd/tools/verify_data_files/main/main.go

+
+	// NOTE: we output to a new directory so that we don't clobber files.
+	writeFsOpts := fsOpts.SetFilePathPrefix(opts.fixDir)
+	writer, err := fs.NewWriter(writeFsOpts)


Should this defer writer.Close() and return nil at the end instead?

Sure thing.

Since writer.Close() does quite a few final steps, I'm going to continue to return an error if it returns an error.

To make sure we always close up I changed it to this:

success := false defer func() { if !success { writer.Close() } }() // do work success = true return writer.Close()

Sounds good 👍

arnikola · 2020-01-05T00:25:50Z

src/cmd/tools/verify_data_files/main/main.go

+				// Need to remove invalid tags.
+				tagsBuffer = tagsBuffer[:0]
+				for _, tag := range currTags {
+					if err := convert.ValidateSeriesTag(tag); err != nil {
+						removedTags++
+						continue
+					}
+					tagsBuffer = append(tagsBuffer, tag)
+				}
+
+				// Make sure we write out the modified tags.
+				writeTags = tagsBuffer


Doesn't this risk 2 series with the same tagset? Should probably skip the entire series instead IMO

Sure thing, can remove the series.

robskillington added 2 commits November 16, 2019 17:35

[dbnode] Validate indexable metric tags before insert into index queue

f01de2f

Revert changes to read_data_files

3ef41b8

arnikola reviewed Nov 16, 2019

View reviewed changes

src/cmd/tools/verify_data_files/main/main.go Outdated

@@ -0,0 +1,398 @@

// Copyright (c) 2018 Uber Technologies, Inc.

Copy link

Collaborator

arnikola Nov 16, 2019

Choose a reason for hiding this comment

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

Nit 19

Fix copyright

8b2d53e

robskillington added 4 commits November 16, 2019 18:15

Add debug log level and fix namespace path resolution

f6bae99

Add readme

2cf09ff

Add test for fixing IDs and tags

d35fcae

Add tests

ee0d66c

robskillington added this to Reviewing in Release v0.15 Jan 3, 2020

robskillington moved this from Reviewing to In progress in Release v0.15 Jan 3, 2020

arnikola reviewed Jan 3, 2020

View reviewed changes

robskillington added 4 commits January 4, 2020 15:36

Merge branch 'master' into r/verify-data-files-tool

c34c22b

Remove verify_commitlogs tool

a9bf5d8

Cleanup errors

4575352

Fix build

f3fa81a

robskillington moved this from In progress to Reviewing in Release v0.15 Jan 4, 2020

robskillington added 2 commits January 4, 2020 17:37

Merge branch 'master' into r/verify-data-files-tool

c31bcde

Test using monorepo queue

1d3806c

arnikola reviewed Jan 4, 2020

View reviewed changes

Revert to workers queue

e2610ee

arnikola reviewed Jan 4, 2020

View reviewed changes

arnikola reviewed Jan 5, 2020

View reviewed changes

Address feedback

9eada43

arnikola approved these changes Jan 5, 2020

View reviewed changes

arnikola moved this from Reviewing to Needs Merge in Release v0.15 Jan 5, 2020

robskillington added 3 commits January 4, 2020 23:58

Make failing tests +big

a112602

Merge branch 'master' into r/verify-data-files-tool

fe18137

Move +big test into dedicated file

6a5832f

robskillington merged commit 5cd41e0 into master Jan 5, 2020

robskillington deleted the r/verify-data-files-tool branch January 5, 2020 05:32

arnikola moved this from Needs Merge to Done in Release v0.15 Jan 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Validate indexable metric tags before insert into index queue #2046

[dbnode] Validate indexable metric tags before insert into index queue #2046

robskillington commented Nov 16, 2019

arnikola Nov 16, 2019

codecov bot commented Nov 16, 2019 •

edited

arnikola Nov 19, 2019

robskillington Jan 4, 2020

arnikola Nov 19, 2019

robskillington Jan 4, 2020

arnikola Nov 19, 2019

arnikola Nov 19, 2019

robskillington Jan 4, 2020

arnikola Nov 19, 2019

robskillington Jan 4, 2020

arnikola Jan 4, 2020

arnikola Jan 4, 2020

robskillington Jan 4, 2020

arnikola Jan 4, 2020

arnikola Jan 4, 2020 •

edited

robskillington Jan 5, 2020

arnikola Jan 4, 2020

robskillington Jan 4, 2020

arnikola Jan 4, 2020

robskillington Jan 5, 2020

arnikola Jan 4, 2020

robskillington Jan 5, 2020

arnikola Jan 4, 2020

robskillington Jan 5, 2020

robskillington Jan 5, 2020

arnikola Jan 5, 2020

arnikola Jan 5, 2020

robskillington Jan 5, 2020

		@@ -0,0 +1,398 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

[dbnode] Validate indexable metric tags before insert into index queue #2046

[dbnode] Validate indexable metric tags before insert into index queue #2046

Conversation

robskillington commented Nov 16, 2019

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2019 • edited

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

arnikola Jan 4, 2020 • edited

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

codecov bot commented Nov 16, 2019 •

edited

arnikola Jan 4, 2020 •

edited