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.

Sign up for GitHub

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

Jump to bottom

Add Graphite query render and find #1308

Merged

arnikola merged 21 commits into master from arnikola/graphite-endpoint

Jan 27, 2019

Collaborator

arnikola commented Jan 19, 2019 •

edited by robskillington

Loading

Adds graphite query endpoint to m3query. This is largely a port from our internal legacy service with some additional transform steps that contain the blast radius to a few codepaths.

TODO:

Readme describing that this code is largely migrated from existing project and has not been modernized
Update graphite string transform to tags to mirror write path
Add globbing detection for queries
Tests for new plumbing, and holes in existing coverage
LTTB downsampling when maxDataPoints is specified

arnikola added 7 commits

January 18, 2019 13:49


          Initial commit of graphite engine code

70ae14d


          WIP 2, bringing over some functions

460af4f


          More changes

da32728


          WIP3

1408fd1


          WIP 4(5?)

68731b2


          tests passing

ee134d3


          Adding files

7a4b6ae

codecov bot commented Jan 20, 2019 •

edited

Loading

Codecov Report

Merging #1308 into master will decrease coverage by 0.1%.
The diff coverage is 68.5%.

@@           Coverage Diff            @@
##           master   #1308     +/-   ##
========================================
- Coverage    70.7%   70.6%   -0.2%     
========================================
  Files         770     811     +41     
  Lines       64764   68943   +4179     
========================================
+ Hits        45824   48681   +2857     
- Misses      15976   17125   +1149     
- Partials     2964    3137    +173

Flag	Coverage Δ
#aggregator	`80.9% <ø> (ø)`	⬆️
#cluster	`85.6% <ø> (ø)`	⬆️
#collector	`78.4% <ø> (ø)`	⬆️
#dbnode	`80.9% <ø> (-0.1%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.6% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.8% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`64% <68.5%> (+2%)`	⬆️
#x	`75.9% <ø> (ø)`	⬆️

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 8a29241...b9e3f37. Read the comment docs.

arnikola added 2 commits

January 21, 2019 18:05


          WIP 6

53e056d


          Cleaning up files

0cb8613

robskillington reviewed

View reviewed changes

src/query/graphite/common/_blacklist_testgo Outdated

		@@ -0,0 +1,89 @@
		package common

Collaborator

robskillington Jan 22, 2019

Think we can delete this file (blacklist)

robskillington reviewed

View reviewed changes

src/query/graphite/common/_blacklistgo Outdated

		@@ -0,0 +1,229 @@
		package common

Collaborator

robskillington Jan 22, 2019

Same with this one.

Collaborator Author

arnikola Jan 22, 2019

Oops forgot to delete the ones in /common/

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/glob_test.go Outdated

+              		{"cities\\+all.'china<1001>'.demarsk", "cities\\+all\\.+\\'china\\<1001\\>\\'\\.+demarsk"},
+              		{"foo.host.me{1,2,3}.*", "foo\\.+host\\.+me(1|2|3)\\.+[^\\.]*"},
+              		{"bar.zed.whatever[0-9].*.*.sjc1", "bar\\.+zed\\.+whatever[0-9]\\.+[^\\.]*\\.+[^\\.]*\\.+sjc1"},
+              		{"optic{0[3-9],1[0-9],20}", "optic(0[3-9]|1[0-9]|20)"},

Collaborator

robskillington Jan 22, 2019

Hm, replace optic with something more generic like foo here?

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/glob_test.go Outdated

+              				"stats.sjc1.timers.rt-geofence.geofence-sjc1.handler.query.count",
+              				"stats.sjc1.timers.rt-geofence.geofence.0-sjc1.handler.query.count",
+              				"stats.sjc1.timers.rt-geofence.geofence021-sjc1.handler.query.count",
+              				"stats.sjc1.timers.rt-geofence.geofence991-sjc1.handler.query.count"}},

Collaborator

robskillington Jan 22, 2019 •

edited

Loading

Replace all these with something more generic, like foo.bar.baz.timers....?

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/glob_test.go Outdated

+              		{"stats.sjc1.timers.rt-geofence.geofence??-sjc1.handler.query.count", true,
+              			[]string{
+              				"stats.sjc1.timers.rt-geofence.geofence01-sjc1.handler.query.count",
+              				"stats.sjc1.timers.rt-geofence.geofence24-sjc1.handler.query.count"}},

Collaborator

robskillington Jan 22, 2019

Similar here, replace all these with something more generic, like foo.bar.baz.timers....?

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/glob_test.go

+              		{"optic{0[3-9],1[0-9],20}", true,
+              			[]string{"optic03", "optic10", "optic20"}},
+              		{"optic{0[3-9],1[0-9],20}", false,
+              			[]string{"optic01", "optic21", "optic201", "optic031"}},

Collaborator

robskillington Jan 22, 2019

Yeah, need to change optic with foo or similar here.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/glob_test.go

+              		{"foo.host{1,2}.zed", false,
+              			[]string{"foo.host3.zed", "foo.hostA.zed", "blad.host1.zed", "foo.host1.zed.z"}},
+              		{"optic{0[3-9],1[0-9],20}", true,
+              			[]string{"optic03", "optic10", "optic20"}},

Collaborator

robskillington Jan 22, 2019

Yeah, need to change optic with foo or similar here.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/graphite_types.go Outdated

+              // FindResultsTreeJSON is graphite's "treeJSON" format for /metrics/find
+              // results.  sample: {"text": "cpus", "expandable": 1, "leaf": 0, "id":
+              // "servers.rtkibana02-sjc1.cpu.cpus", "allowChildren": 1}

Collaborator

robskillington Jan 22, 2019

Yeah change servers.rtkibana02-sjc1.cpu.cpus to something like servers.foo.cpu.cpus in this example.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/graphite_types.go Outdated

+              // MetricsPathMetadata is an internal element of graphite's "completer" format
+              // for /metrics/find results.  sample: {"is_leaf": "1", "path":
+              // "servers.rtkibana02-sjc1.cpu.context_switches", "name":

Collaborator

robskillington Jan 22, 2019 •

edited

Loading

Yeah change servers.rtkibana02-sjc1.cpu.context_switches to something like servers.foo.cpu.context_switches in this example.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/graphite_types_test.go Outdated

+              	err = json.Unmarshal(data, &results)
+              	require.Nil(t, err)
+              	require.Equal(t, 1, len(results))
+              	require.Equal(t, "exp.sjc1.timers.cn.production.cn37-sjc1.city_load.monterrey.total.p95", results[0].Target)

Collaborator

robskillington Jan 22, 2019

We need to replace all IDs in testdata with more generic, non-real IDs.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/identify_test.go Outdated

+              const (
+              	statsdStat    = "dispatch.production.san_francisco.uberx.drivers"
+              	malformedStat = "dispatchproductionsan_franciscouberxdrivers"

Collaborator

robskillington Jan 22, 2019

Similar, need to change both these two stats to more test data-ish looking names.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/identify_test.go Outdated

+              }
+              func TestExtractNthMetricPartNoDots(t *testing.T) {
+              	nodots := "dispatchproductionsan_franciscouberxdrivers"

Collaborator

robskillington Jan 22, 2019

Similar, need to change both these two stats to more test data-ish looking names.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/identify_test.go Outdated

+              func TestExtractNthMetricPartStandardCase(t *testing.T) {
+              	assert.Equal(t, "dispatch", ExtractNthMetricPart(statsdStat, 0))
+              	assert.Equal(t, "production", ExtractNthMetricPart(statsdStat, 1))
+              	assert.Equal(t, "drivers", ExtractNthMetricPart(statsdStat, 4))

Collaborator

robskillington Jan 22, 2019

Similar, need to change both these two stats to more test data-ish looking names.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/metric_type_test.go Outdated

+              	assert.Equal(t, metric.UnknownType, MetricTypeFromID("servers.m3-kv05-dca1.disk.sdd.rkb"))
+              	assert.Equal(t, metric.CounterType, MetricTypeFromID("stats.sjc1.counts.m3+calls+dc=sjc1"))
+              	assert.Equal(t, metric.GaugeType, MetricTypeFromID("stats.sjc1.gauges.m3+task+dc=sjc1"))
+              	assert.Equal(t, metric.TimerType, MetricTypeFromID("stats.sjc1.timers.haproxy.beagle.response_time.p99"))

Collaborator

robskillington Jan 22, 2019 •

edited

Loading

Similar, need to change all these two stats to more test data-ish looking names.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/metric_type.go Outdated

		@@ -0,0 +1,117 @@
		package graphite

		import (

Collaborator

robskillington Jan 22, 2019

Actually wonder if we need the metric type detection at all, think we can possibly do without it.

Let me see where this tricks up from.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/retention_policies.go Outdated

+              var (
+              	// NB(mmihic): These need to be ordered
+              	graphiteRetentionPeriods = []*RetentionPeriod{

Collaborator

robskillington Jan 22, 2019

Don't think we need any of these retention period definitions.

Let me see where this tricks up from, I'm pretty certain we can remove them.

robskillington reviewed

View reviewed changes

src/query/graphite/graphite/testdata/no-results.json Outdated

		@@ -0,0 +1 @@
		[{"target":"exp.sjc1.timers.cn.production.cn37-sjc1.city_load.monterrey.total.p95","datapoints":[[null,1430835420],[null,1430835430],[null,1430835440],[null,1430835450],[null,1430835460],[null,1430835470],[null,1430835480],[null,1430835490],[null,1430835500],[null,1430835510],[null,1430835520],[null,1430835530],[null,1430835540],[null,1430835550],[null,1430835560],[null,1430835570],[null,1430835580],[null,1430835590],[null,1430835600],[null,1430835610],[null,1430835620],[null,1430835630],[null,1430835640],[null,1430835650],[null,1430835660],[null,1430835670],[null,1430835680],[null,1430835690],[null,1430835700],[null,1430835710],[null,1430835720],[null,1430835730],[null,1430835740],[null,1430835750],[null,1430835760],[null,1430835770],[null,1430835780],[null,1430835790],[null,1430835800],[null,1430835810],[null,1430835820],[null,1430835830],[null,1430835840],[null,1430835850],[null,1430835860],[null,1430835870],[null,1430835880],[null,1430835890],[null,1430835900],[null,1430835910],[null,1430835920],[null,1430835930],[null,1430835940],[null,1430835950],[null,1430835960],[null,1430835970],[null,1430835980],[null,1430835990],[null,1430836000],[null,1430836010],[null,1430836020],[null,1430836030],[null,1430836040],[null,1430836050],[null,1430836060],[null,1430836070],[null,1430836080],[null,1430836090],[null,1430836100],[null,1430836110],[null,1430836120],[null,1430836130],[null,1430836140],[null,1430836150],[null,1430836160],[null,1430836170],[null,1430836180],[null,1430836190],[null,1430836200],[null,1430836210],[null,1430836220],[null,1430836230],[null,1430836240],[null,1430836250],[null,1430836260],[null,1430836270],[null,1430836280],[null,1430836290],[null,1430836300],[null,1430836310],[null,1430836320],[null,1430836330],[null,1430836340],[null,1430836350],[null,1430836360],[null,1430836370],[null,1430836380],[null,1430836390],[null,1430836400],[null,1430836410],[null,1430836420],[null,1430836430],[null,1430836440],[null,1430836450],[null,1430836460],[null,1430836470],[null,1430836480],[null,1430836490],[null,1430836500],[null,1430836510],[null,1430836520],[null,1430836530],[null,1430836540],[null,1430836550],[null,1430836560],[null,1430836570],[null,1430836580],[null,1430836590],[null,1430836600],[null,1430836610],[null,1430836620],[null,1430836630],[null,1430836640],[null,1430836650],[null,1430836660],[null,1430836670],[null,1430836680],[null,1430836690],[null,1430836700],[null,1430836710],[null,1430836720],[null,1430836730],[null,1430836740],[null,1430836750],[null,1430836760],[null,1430836770],[null,1430836780],[null,1430836790],[null,1430836800],[null,1430836810],[null,1430836820],[null,1430836830],[null,1430836840],[null,1430836850],[null,1430836860],[null,1430836870],[null,1430836880],[null,1430836890],[null,1430836900],[null,1430836910],[null,1430836920],[null,1430836930],[null,1430836940],[null,1430836950],[null,1430836960],[null,1430836970],[null,1430836980],[null,1430836990],[null,1430837000],[null,1430837010],[null,1430837020],[null,1430837030],[null,1430837040],[null,1430837050],[null,1430837060],[null,1430837070],[null,1430837080],[null,1430837090],[null,1430837100],[null,1430837110],[null,1430837120],[null,1430837130],[null,1430837140],[null,1430837150],[null,1430837160],[null,1430837170],[null,1430837180],[null,1430837190],[null,1430837200],[null,1430837210],[null,1430837220],[null,1430837230],[null,1430837240],[null,1430837250],[null,1430837260],[null,1430837270],[null,1430837280],[null,1430837290],[null,1430837300],[null,1430837310],[null,1430837320],[null,1430837330],[null,1430837340],[null,1430837350],[null,1430837360],[null,1430837370],[null,1430837380],[null,1430837390],[null,1430837400],[null,1430837410],[null,1430837420],[null,1430837430],[null,1430837440],[null,1430837450],[null,1430837460],[null,1430837470],[null,1430837480],[null,1430837490],[null,1430837500],[null,1430837510],[null,1430837520],[null,1430837530],[null,1430837540],[null,1430837550],[null,1430837560],[null,1430837570],[null,1430837580],[null,1430837590],[null,1430837600],[null,1430837610],[null,1430837620],[null,1430837630],[null,1430837640],[null,1430837650],[null,1430837660],[null,1430837670],[null,1430837680],[null,1430837690],[null,1430837700],[null,1430837710],[null,1430837720],[null,1430837730],[null,1430837740],[null,1430837750],[null,1430837760],[null,1430837770],[null,1430837780],[null,1430837790],[null,1430837800],[null,1430837810],[null,1430837820],[null,1430837830],[null,1430837840],[null,1430837850],[null,1430837860],[null,1430837870],[null,1430837880],[null,1430837890],[null,1430837900],[null,1430837910],[null,1430837920],[null,1430837930],[null,1430837940],[null,1430837950],[null,1430837960],[null,1430837970],[null,1430837980],[null,1430837990],[null,1430838000],[null,1430838010],[null,1430838020],[null,1430838030],[null,1430838040],[null,1430838050],[null,1430838060],[null,1430838070],[null,1430838080],[null,1430838090],[null,1430838100],[null,1430838110],[null,1430838120],[null,1430838130],[null,1430838140],[null,1430838150],[null,1430838160],[null,1430838170],[null,1430838180],[null,1430838190],[null,1430838200],[null,1430838210],[null,1430838220],[null,1430838230],[null,1430838240],[null,1430838250],[null,1430838260],[null,1430838270],[null,1430838280],[null,1430838290],[null,1430838300],[null,1430838310],[null,1430838320],[null,1430838330],[null,1430838340],[null,1430838350],[null,1430838360],[null,1430838370],[null,1430838380],[null,1430838390],[null,1430838400],[null,1430838410],[null,1430838420],[null,1430838430],[null,1430838440],[null,1430838450],[null,1430838460],[null,1430838470],[null,1430838480],[null,1430838490],[null,1430838500],[null,1430838510],[null,1430838520],[null,1430838530],[null,1430838540],[null,1430838550],[null,1430838560],[null,1430838570],[null,1430838580],[null,1430838590],[null,1430838600],[null,1430838610],[null,1430838620],[null,1430838630],[null,1430838640],[null,1430838650],[null,1430838660],[null,1430838670],[null,1430838680],[null,1430838690],[null,1430838700],[null,1430838710],[null,1430838720],[null,1430838730],[null,1430838740],[null,1430838750],[null,1430838760],[null,1430838770],[null,1430838780],[null,1430838790],[null,1430838800],[null,1430838810],[null,1430838820],[null,1430838830],[null,1430838840],[null,1430838850],[null,1430838860],[null,1430838870],[null,1430838880],[null,1430838890],[null,1430838900],[null,1430838910],[null,1430838920],[null,1430838930],[null,1430838940],[null,1430838950],[null,1430838960],[null,1430838970],[null,1430838980],[null,1430838990],[null,1430839000],[null,1430839010]]}]

Collaborator

robskillington Jan 22, 2019

Yeah need to update this metric ID in here to be more test data-ish.

robskillington reviewed

View reviewed changes

src/query/graphite/lexer/lexer_test.go

+              	reservedIdents map[string]TokenType
+              }
+              func TestLexer(t *testing.T) {

Collaborator

robskillington Jan 22, 2019

Similar for this method, need to remove all of the non-test data IDs and make them more synthetic/test data like (e.g. foo/bar/baz/etc)

robskillington reviewed

View reviewed changes

src/query/graphite/ts/series.go Outdated

+              	Specification string
+              	// Metric tags.
+              	Tags map[string]string

Collaborator

robskillington Jan 22, 2019

We can get rid of tags since this is only relevant to graphite.

Collaborator Author

arnikola Jan 22, 2019

It's a bit more involved since it's used in a bunch of tests, might do a more specialized diff to get rid of it

robskillington reviewed

View reviewed changes

src/query/graphite/ts/series.go

+              	GraphOptions map[string]string
+              	// consolidationFunc specifies how the series will be consolidated when the
+              	// number of data points in the series is more than the maximum number allowed.

Collaborator

robskillington Jan 22, 2019

We can get rid of GraphOptions, this isn't relevant for a graphite only implementation.

arnikola added 3 commits

January 22, 2019 11:32


          Sanitize values, further cleanup

f05b505


          Removed unnecessary fields on graphite series

051deba


          Add end-to-end handler, ready for proper review

327aaee

arnikola changed the title ~~[WIP] [query] Add graphite querying~~ [query] Add graphite query path

arnikola requested review from nikunjgit, robskillington, richardartoul, benraskin92 and andrewmains12

January 22, 2019 21:58

arnikola added 6 commits

January 23, 2019 17:50


          Updated graphite string transform

14c8de1

Added graphite glob detection
Added tests for new plumbing


          Further cleanup

437c319


          Adds search endpoint


          Changed return format for search endpoint.

030f2c5


          Merge branch 'master' into arnikola/graphite-endpoint

1d36e6e


          Merged and replaced tag generation with ingestion approach.

402c51d

robskillington reviewed

View reviewed changes

src/query/api/v1/handler/graphite/native/render_parser.go Outdated

+              	if isRealTimeQuery && isLargeRangeQuery {
+              		p.From = p.From.Add(queryRangeShift)
+              		p.Until = p.Until.Add(queryRangeShift)
+              	}

Collaborator

robskillington Jan 26, 2019 •

edited

Loading

We can probably just get rid of all this for now. (the shifting for making it more likely to return high resolution data)

robskillington reviewed

View reviewed changes

src/query/api/v1/handler/graphite/native/search.go Outdated

+              const (
+              	// GraphiteSearchURL is the url for searching graphite paths.
+              	GraphiteSearchURL = handler.RoutePrefixGraphiteV1 + "/search"

Collaborator

robskillington Jan 26, 2019

Think this has to be /find.

robskillington reviewed

View reviewed changes

src/query/api/v1/handler/graphite/native/search.go Outdated

+              		value := tags[index].Value
+              		// If this value has already been encountered, check if
+              		if hadExtra, seen := seenMap[string(value)]; seen {
+              			if hadExtra {

Collaborator

robskillington Jan 26, 2019

Combine these two if's perhaps?

if hadExtra, seen := seenMap[string(value)]; seen && hadExtra {
  continue
}

robskillington reviewed

View reviewed changes

src/query/graphite/common/context.go Outdated

+              	Engine    QueryEngine
+              	LocalOnly bool
+              	UseCache  bool
+              	UseM3DB   bool

Collaborator

robskillington Jan 26, 2019

Can remove LocalOnly, UseCache and UseM3DB here.

Collaborator Author

arnikola Jan 26, 2019

Ah damn, removed them from regular context object but missed cleaning up contextBase

robskillington reviewed

View reviewed changes

src/query/graphite/common/context.go Outdated

+              	UseCache  bool
+              	UseM3DB   bool
+              	Timeout   time.Duration
+              	Zone      string

Collaborator

robskillington Jan 26, 2019

Can also remove Zone.

Rob Skillington added 3 commits

January 26, 2019 00:18


          Fixes for Grafana support

eea190c


          Rename series back into graphite names, set values at correct value i…

6f7c63b

…ndex based on timestamp


          Add LTTB downsampling when using maxDataPoints and fix tests

b9e3f37

Collaborator

robskillington commented Jan 27, 2019

All tests are passing, but FOSSA upload is failing, probably because of some service interruption it looks like. Let's re-run that in a bit.

robskillington approved these changes

View reviewed changes

Collaborator

robskillington left a comment

LGTM

arnikola merged commit 8685af8 into master

arnikola deleted the arnikola/graphite-endpoint branch

January 27, 2019 19:57

robskillington pushed a commit that referenced this pull request


          [query] Add graphite query path (#1308)

78a776c

Adds processing for carbon queries, handling the find and query endpoints.

robskillington changed the title ~~[query] Add graphite query path~~ Add Graphite query render and find support

robskillington changed the title ~~Add Graphite query render and find support~~ Add Graphite query render and find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

robskillington robskillington approved these changes

nikunjgit Awaiting requested review from nikunjgit

richardartoul Awaiting requested review from richardartoul

benraskin92 Awaiting requested review from benraskin92

andrewmains12 Awaiting requested review from andrewmains12

Labels

None yet