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

Added ES SimpleClient support for bosun backend and annotation. #1947

Merged
merged 4 commits into from Apr 24, 2017

Conversation

@pradeepbbl
Copy link
Contributor

@pradeepbbl pradeepbbl commented Oct 24, 2016

This change will allow to create a light weight elastic search client which is suitable for elasticsearch standalone server.

@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 24, 2016

Test is failing due to missing annotation backend changes bosun-monitor/annotate#6 are not in place.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 2 times, most recently from 36867f8 to caddacf Oct 24, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 24, 2016

@captncraig after vendor update getting git conflicts

@captncraig
Copy link
Contributor

@captncraig captncraig commented Oct 24, 2016

Oh crap. Yeah, that sucks. Try git checkout --ours vendor/vendor.json and see if that looks ok. (after git merge origin/master)

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 3 times, most recently from c1e678e to ca37c05 Oct 24, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 24, 2016

@captncraig I tired everything to get get rid of vendor/vendor.json conflict ..am able to merge it in my local repo but it's still failing here. I will retry tom, mean while if you have some free time have look at my version and let me know if you find anything suspicious.

@captncraig
Copy link
Contributor

@captncraig captncraig commented Oct 24, 2016

Here's what I'd recommend.

  1. remove all vendor related changes from this branch.
  2. rebase onto origin/master
  3. make all vendor changes over again.

Sorry, I know this can be real painful.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 4 times, most recently from 00143f3 to 415bbb9 Oct 25, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 25, 2016

@captncraig finally am able to fix the vendor.json conflict issue by replacing the 'revision' hash and 'revisionTime' from origin/master not sure it was the right thing to do?

screen shot 2016-10-25 at 11 48 57 am

During the merging process I have noticed go format issue in backend/backend.go, pls have look at bosun-monitor/annotate#7

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch from 415bbb9 to 1222f1e Oct 25, 2016
@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Oct 25, 2016

Here is what I do when working on annotate and vendoring it:

govendor remove github.com/bosun-monitor/annotate/... && govendor add github.com/bosun-monitor/annotate && govendor add github.com/bosun-monitor/annotate/backend && govendor add github.com/bosun-monitor/annotate/web && go run main.go -c bosun.toml -w -r -q -dev -skiplast -n

I think maybe because gorilla stuff things need to be added in a certain
order or something. But the above has always worked for me.

On Tue, Oct 25, 2016 at 5:53 AM, Pradeep Mishra notifications@github.com
wrote:

@captncraig https://github.com/captncraig finally am able to fix the
vendor.json conflict issue by replacing the 'revision' hash and
'revisionTime' from origin/master not sure it was the right thing to do?

[image: screen shot 2016-10-25 at 11 48 57 am]
https://cloud.githubusercontent.com/assets/6874494/19681284/168dd208-9aa9-11e6-9ef6-4e44d73f4390.png

During the merging process I have noticed go format issue in
backend/backend.go, pls have look at bosun-monitor/annotate#7
bosun-monitor/annotate#7


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1947 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABnT0McgMZvNUKSM4kKceLCkvHWoP6tEks5q3dGegaJpZM4Ke4MM
.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch from 1222f1e to c49681e Oct 26, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 26, 2016

this is dependent on bosun-monitor/annotate#7

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch from c49681e to b752093 Oct 26, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Oct 26, 2016

force updated vendor/github.com/bosun-monitor/annotate/backend/backend.go by manually replacing the file, which isn't the right way to do but want to make sure all checks are passed.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 4 times, most recently from be71172 to 11263d2 Oct 27, 2016
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Feb 17, 2017

@kylebrandt done with code merge for multi elastic backend support :), do let me know if you find any issue or require any further changes.

#1404

Thanks,

@pradeepbbl pradeepbbl changed the title Added ES SimpleClient support for bosun backend and annotation. Added ES SimpleClient support for bosun backend and annotation. eedcab9 #1404 Feb 17, 2017
@pradeepbbl pradeepbbl changed the title Added ES SimpleClient support for bosun backend and annotation. eedcab9 #1404 Added ES SimpleClient support for bosun backend and annotation. Feb 17, 2017
@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Feb 27, 2017

Need to look at the code, but I think it might be better to assign which backend to use to the Context object attached to each template. That way we don't have to change the arguments to the function. This may not matter so much for the elastic functions, but when we want to add the functionality to more tsdb providers than they won't have to change either.

Since templates are procedural, this should work. You would just do something like `{{ .ES = "mySecondary"}}. Or it could be a method on the context like {{ .UseElastic "mySecondary" }}

@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Mar 2, 2017

@kylebrandt if we move the variable out from functions to context object expression checks using /expr page are failing with empty prefix key e.g escount($index, $keyField, $filter, $bucketSize, "3h", ""), are you okay to refernece the context obj in expr.go

@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Mar 6, 2017

@pradeepbbl I don't understand why it would need to access the context object in the expr package?

Can't you just call the functions passing the key from the context instead of an argument to the function?

func (c *Context) ESQuery(indexRoot expr.ESIndexer, filter expr.ESQuery, sduration, eduration string, size int, key string) (interface{}, error) {
  	newFilter := expr.ScopeES(c.Group(), filter.Query)		  	newFilter := expr.ScopeES(c.Group(), filter.Query)
 -	req, err := expr.ESBaseQuery(c.runHistory.Start, indexRoot, newFilter, sduration, eduration, size)		 +	req, err := expr.ESBaseQuery(c.runHistory.Start, indexRoot, newFilter, sduration, eduration, size, c.ElasticHost)
...

and then something like:

template test {
	subject = {{.Last.Status}}: {{.Alert.Name}} on {{.Group.host}}
	body = `
	    {{ $filter := (.Eval .Alert.Vars.filter)}}
	    {{ $index := (.Eval .Alert.Vars.index)}}
	    {{ $hostkey := (.Eval .Alert.Vars.prefixkey)}}
            {{ .UseElastic "mySecondary" }}
	    {{range $i, $x := .ESQuery $index $filter "5m" "" 10}}
	        <p>{{$x.machinename}}</p>
	    {{end}}
	`
}
@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Mar 7, 2017

Also, could it be make so the text inside [ ] has to be a quoted string? So [foo] would be invalid but ["foo"] would be valid. Even if we fake it for now and don't actually treat the contents as a string token, at least people won't have differing syntax in their configs should we decide to do so in the future.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch from e775a8d to 6b38c88 Apr 3, 2017
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Apr 3, 2017

rebase with 'upstream/master'

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 4 times, most recently from 7445cf5 to c7b6781 Apr 11, 2017
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Apr 12, 2017

@kylebrandt done with the merging and changes discussed above. Please have a look and let me know if anything need to be changed.

Test's Done:

  • Merging two different query with and without prefix : pass
  • Tested prefix key with double quotes e.g ["foo"]: pass
  • Tested prefix key without double quotes e.g [foo]: failed (expected result)

Thanks,

@@ -41,6 +44,7 @@ func (s *Schedule) Data(rh *RunHistory, st *models.IncidentState, a *conf.Alert,
IsEmail: isEmail,
schedule: s,
runHistory: rh,
ElasticHost: es,

This comment has been minimized.

@kylebrandt

kylebrandt Apr 18, 2017
Member

I think we can get rid of the global var here. Initalize it to the string "default" like in https://github.com/bosun-monitor/bosun/pull/1947/files#diff-d6bd3827d8c8d679e462f07cef34f092R755. The for UseElastic have it set c.ElasticHost (and doesn't need to return anything). Since UseElastic is a method on a pointer it will change the context object.

This comment has been minimized.

@pradeepbbl

pradeepbbl Apr 18, 2017
Author Contributor

Sure, I have added it for debug purpose. I will make the change as suggested.

I would also like your suggestion on logging ES host map https://github.com/bosun-monitor/bosun/pull/1947/files#diff-2039e8e0c0f4873ec699c321c1e336f9R261, are you okay with it?

}
var err error
if e.PrefixKey != "" {
slog.Infof("es prefix key found connecting to host: %s", e.Hosts[e.PrefixKey])

This comment has been minimized.

@kylebrandt

kylebrandt Apr 18, 2017
Member

this should be removed since it logs whenever the expressions are executed so it will be spammy.

slog.Infof("es prefix key found connecting to host: %s", e.Hosts[e.PrefixKey])
} else {
e.PrefixKey = "default"
slog.Infof("es prefix key missing set it to default")

This comment has been minimized.

@kylebrandt

kylebrandt Apr 18, 2017
Member

and remove this one as well.

@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Apr 18, 2017

Oh and documentation. Need to update docs/expression.md to explain the elastic prefix , docs/system_configuration.md with the changes to ES configuration, and definitions.md with the new template func.

@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch 2 times, most recently from 8574445 to c3e3c80 Apr 18, 2017
@pradeepbbl
Copy link
Contributor Author

@pradeepbbl pradeepbbl commented Apr 19, 2017

done with the documentation :)

@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Apr 19, 2017

https://github.com/bosun-monitor/bosun/tree/prefixIdea illustrates what I think might be a better way to pass the prefix.

Diff:

diff --git a/cmd/bosun/expr/elastic.go b/cmd/bosun/expr/elastic.go
index 547c74c..aa7948a 100644
--- a/cmd/bosun/expr/elastic.go
+++ b/cmd/bosun/expr/elastic.go
@@ -9,6 +9,7 @@ import (
 	"bosun.org/cmd/bosun/expr/parse"
 	"bosun.org/models"
 	"bosun.org/opentsdb"
+	"bosun.org/slog"
 	"github.com/MiniProfiler/go/miniprofiler"
 	"github.com/jinzhu/now"
 	elastic "gopkg.in/olivere/elastic.v3"
@@ -32,10 +33,11 @@ func elasticTagQuery(args []parse.Node) (parse.Tags, error) {
 var Elastic = map[string]parse.Func{
 	// Funcs for querying elastic
 	"escount": {
-		Args:   []models.FuncType{models.TypeESIndexer, models.TypeString, models.TypeESQuery, models.TypeString, models.TypeString, models.TypeString},
-		Return: models.TypeSeriesSet,
-		Tags:   elasticTagQuery,
-		F:      ESCount,
+		Args:          []models.FuncType{models.TypeESIndexer, models.TypeString, models.TypeESQuery, models.TypeString, models.TypeString, models.TypeString},
+		Return:        models.TypeSeriesSet,
+		Tags:          elasticTagQuery,
+		F:             ESCount,
+		PrefixEnabled: true,
 	},
 	"esstat": {
 		Args:   []models.FuncType{models.TypeESIndexer, models.TypeString, models.TypeESQuery, models.TypeString, models.TypeString, models.TypeString, models.TypeString, models.TypeString},
@@ -419,7 +421,8 @@ func ESMonthly(e *State, T miniprofiler.Timer, timeField, indexRoot, layout stri
 	return &r, nil
 }
 
-func ESCount(e *State, T miniprofiler.Timer, indexer ESIndexer, keystring string, filter ESQuery, interval, sduration, eduration string) (r *Results, err error) {
+func ESCount(prefix string, e *State, T miniprofiler.Timer, indexer ESIndexer, keystring string, filter ESQuery, interval, sduration, eduration string) (r *Results, err error) {
+	slog.Infoln("Prefix: ", prefix)
 	return ESDateHistogram(e, T, indexer, keystring, filter.Query, interval, sduration, eduration, "", "", 0)
 }
 
diff --git a/cmd/bosun/expr/expr.go b/cmd/bosun/expr/expr.go
index 594d6f2..f1d2394 100644
--- a/cmd/bosun/expr/expr.go
+++ b/cmd/bosun/expr/expr.go
@@ -715,9 +715,14 @@ func (e *State) walkPrefix(node *parse.PrefixNode, T miniprofiler.Timer) *Result
 	key, _ = strconv.Unquote(key)
 	switch node := node.Arg.(type) {
 	case *parse.FuncNode:
+		// TODO. Change this to be more generic by having some sort of "supports prefix"
+		// also perhaps better as part of check IIRC (look later)
 		if strings.Contains(node.Name, "es") {
-			e.ElasticHosts.PrefixKey = key
-			prefixkey = true
+			// e.ElasticHosts.PrefixKey = key
+			// prefixkey = true
+
+			// Set the prefix on the func node
+			node.Prefix = key
 		}
 		return e.walk(node, T)
 	default:
@@ -772,7 +777,12 @@ func (e *State) walkFunc(node *parse.FuncNode, T miniprofiler.Timer) *Results {
 		}
 
 		f := reflect.ValueOf(node.F.F)
-		fr := f.Call(append([]reflect.Value{reflect.ValueOf(e), reflect.ValueOf(T)}, in...))
+		fr := []reflect.Value{}
+		if node.F.PrefixEnabled {
+			fr = f.Call(append([]reflect.Value{reflect.ValueOf(node.Prefix), reflect.ValueOf(e), reflect.ValueOf(T)}, in...))
+		} else {
+			fr = f.Call(append([]reflect.Value{reflect.ValueOf(e), reflect.ValueOf(T)}, in...))
+		}
 		res = fr[0].Interface().(*Results)
 		if len(fr) > 1 && !fr[1].IsNil() {
 			err := fr[1].Interface().(error)
diff --git a/cmd/bosun/expr/parse/node.go b/cmd/bosun/expr/parse/node.go
index cc15ee8..1fc6008 100644
--- a/cmd/bosun/expr/parse/node.go
+++ b/cmd/bosun/expr/parse/node.go
@@ -72,6 +72,8 @@ type FuncNode struct {
 	Name string
 	F    Func
 	Args []Node
+	// Prefix is comes from the preceeding prefix node
+	Prefix string
 }
 
 func newFunc(pos Pos, name string, f Func) *FuncNode {
diff --git a/cmd/bosun/expr/parse/parse.go b/cmd/bosun/expr/parse/parse.go
index d71acaf..8f277f9 100644
--- a/cmd/bosun/expr/parse/parse.go
+++ b/cmd/bosun/expr/parse/parse.go
@@ -40,6 +40,7 @@ type Func struct {
 	VArgsPos  int
 	VArgsOmit bool
 	MapFunc   bool // Func is only valid in map expressions
+	PrefixEnabled bool
 	Check     func(*Tree, *FuncNode) error
 }

From Slack private chat:

So if you define a PrefixEnabled: true in map[string]parse.Func then it knows to call F: with the prefix as the first argument

[11:47] 
When parsing the prefix, it passes the value to the funcnode for later reference

[11:49] 
What I didn’t do, is make it so `func (f *FuncNode) Check(t *Tree) error {` checks the FunName against PrefixEnabled

[11:49] 
But that will let you get rid of the if “es”
Pradeep Mishra
@pradeepbbl pradeepbbl force-pushed the bookingcom:es-config branch from c3e3c80 to 37c0308 Apr 19, 2017
@kylebrandt
Copy link
Member

@kylebrandt kylebrandt commented Apr 20, 2017

couple more changes at https://github.com/bosun-monitor/bosun/tree/mesFixes

I need to make sure simple client works with these changes though and test a couple other things. Ironically, one of my ES clusters is down right now.. :-/ Should be fixed in an hour or so

diff --git a/cmd/bosun/expr/elastic.go b/cmd/bosun/expr/elastic.go
index 2d3edab5..ae401b57 100644
--- a/cmd/bosun/expr/elastic.go
+++ b/cmd/bosun/expr/elastic.go
@@ -14,8 +14,12 @@ import (
 	elastic "gopkg.in/olivere/elastic.v3"
 )
 
-// This uses a global client since the elastic client handles connections
-var esClient *elastic.Client
+// Map of prefixes to corresponding clients
+var esClients map[string]*elastic.Client
+
+func init() {
+	esClients = make(map[string]*elastic.Client)
+}
 
 func elasticTagQuery(args []parse.Node) (parse.Tags, error) {
 	n := args[1].(*parse.StringNode)
@@ -252,18 +256,33 @@ type ElasticConfig struct {
 }
 
 // InitClient sets up the elastic client. If the client has already been
-// initalized it is a noop
+// initialized it is a noop
 func (e ElasticHosts) InitClient(prefix string) error {
+	if _, ok := e.Hosts[prefix]; !ok {
+		prefixes := make([]string, len(e.Hosts))
+		i := 0
+		for k := range e.Hosts {
+			prefixes[i] = k
+			i++
+		}
+		return fmt.Errorf("prefix %v not defined, available prefixes are: %v", prefix, prefixes)
+	}
+
+	// TODO? Since SimpleClient isn't "long lived", does that need to be created each time?
+	if c := esClients[prefix]; c != nil {
+		// client already initialized
+		return nil
+	}
 	var err error
 	if e.Hosts[prefix].SimpleClient {
 		// simple client enabled
-		esClient, err = elastic.NewSimpleClient(elastic.SetURL(e.Hosts[prefix].Hosts...), elastic.SetMaxRetries(10))
+		esClients[prefix], err = elastic.NewSimpleClient(elastic.SetURL(e.Hosts[prefix].Hosts...), elastic.SetMaxRetries(10))
 	} else if len(e.Hosts[prefix].Hosts) == 0 {
 		// client option enabled
-		esClient, err = elastic.NewClient(e.Hosts[prefix].ClientOptionFuncs...)
+		esClients[prefix], err = elastic.NewClient(e.Hosts[prefix].ClientOptionFuncs...)
 	} else {
 		// default behavior
-		esClient, err = elastic.NewClient(elastic.SetURL(e.Hosts[prefix].Hosts...), elastic.SetMaxRetries(10))
+		esClients[prefix], err = elastic.NewClient(elastic.SetURL(e.Hosts[prefix].Hosts...), elastic.SetMaxRetries(10))
 	}
 
 	if err != nil {
@@ -278,7 +297,7 @@ func (e *ElasticHosts) getService(prefix string) (*elastic.SearchService, error)
 	if err != nil {
 		return nil, err
 	}
-	return esClient.Search(), nil
+	return esClients[prefix].Search(), nil
 }
 
 // Query takes a Logstash request, applies it a search service, and then queries
@@ -288,9 +307,6 @@ func (e ElasticHosts) Query(r *ElasticRequest) (*elastic.SearchResult, error) {
 	if err != nil {
 		return nil, err
 	}
-	if err != nil {
-		return nil, err
-	}
 
 	s.Index(r.Indices...)
 
diff --git a/cmd/bosun/expr/parse/node.go b/cmd/bosun/expr/parse/node.go
index 0377dd0f..e7b386a8 100644
--- a/cmd/bosun/expr/parse/node.go
+++ b/cmd/bosun/expr/parse/node.go
@@ -317,14 +317,13 @@ func (p *PrefixNode) StringAST() string {
 }
 
 func (p *PrefixNode) Check(t *Tree) error {
-	rt := p.Arg.Return()
-
-	if !(rt == models.TypeSeriesSet) {
-		return fmt.Errorf("parse: type error in %s, expected %s, got %s", p, "prefix", rt)
+	if p.Arg.Type() != NodeFunc {
+		return fmt.Errorf("parse: prefix on non-function")
+	}
+	if !p.Arg.(*FuncNode).F.PrefixEnabled {
+		return fmt.Errorf("func %v does not support a prefix", p.Arg.(*FuncNode).Name)
 	}
-
 	return p.Arg.Check(t)
-
 }
 
 func (p *PrefixNode) Return() models.FuncType {

kylebrandt and others added 2 commits Apr 21, 2017
Mes fixes
@kylebrandt kylebrandt merged commit 1f4d85e into bosun-monitor:master Apr 24, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mvuets mvuets deleted the bookingcom:es-config branch Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants