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
cmd/bosun: Azure Monitor Datasource #2283
Conversation
e22f47c
to
1fc3f64
Compare
For review except the metric metadata function which can be ignored for now. Also will vendor later. cc @mhenderson-so . |
cmd/bosun/conf/system.go
Outdated
DebugResponse bool | ||
} | ||
|
||
// Valid returns if the configuration for the AzureMonitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix comment
cmd/bosun/conf/system.go
Outdated
return allClients | ||
} | ||
|
||
func azureLogRequest() autorest.PrepareDecorator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment this function and the following, add comment to attribute source of readme from the go azure sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these even required long term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they hurt, and since the documentation is marking the experimental for now they could be handy. I would probably be better if more datasources had this I think
cmd/bosun/conf/system.go
Outdated
if err != nil { | ||
slog.Warningf("failure to dump azure request: %v", err) | ||
} | ||
dump, _ := httputil.DumpRequestOut(r, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check + log err
cmd/bosun/conf/system.go
Outdated
if err != nil { | ||
slog.Warningf("failure to dump azure request: %v", err) | ||
} | ||
dump, _ := httputil.DumpResponse(r, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check + log err
cmd/bosun/expr/azure.go
Outdated
}, | ||
} | ||
|
||
// Tag function for the "az" expression function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: azTags is
cmd/bosun/expr/azure.go
Outdated
return azureTags(args[2]) | ||
} | ||
|
||
// Tag function for the "azmulti" expression function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: azMultiTags is
cmd/bosun/expr/azure.go
Outdated
|
||
// azureTags adds tags for the csv argument along with the "name" and "rsg" tags | ||
func azureTags(arg parse.Node) (parse.Tags, error) { | ||
tags := parse.Tags{"name": struct{}{}, "rsg": struct{}{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make name and rsg constants and change in all places where referenced
cmd/bosun/expr/azure.go
Outdated
|
||
const azTimeFmt = "2006-01-02T15:04:05" | ||
|
||
func azResourceURI(subscription, resourceGrp, Namespace, Resource string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment func
return | ||
} | ||
|
||
// AzureQuery queries an Azure monitor metric for the given resource and returns a series set tagged by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finish this func comment
cmd/bosun/expr/azure.go
Outdated
// Verify prefix is a defined resource and fetch the collection of clients | ||
cc, clientFound := e.Backends.AzureMonitor[prefix] | ||
if !clientFound { | ||
return r, fmt.Errorf("azure client with name %v not defined", prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... "%v" ...
cmd/bosun/expr/azure.go
Outdated
st := e.now.Add(time.Duration(-sd)).Format(azTimeFmt) | ||
en := e.now.Add(time.Duration(-ed)).Format(azTimeFmt) | ||
|
||
// Set Dimensions (tag) keys for metrics that support them by building a filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expand on comment , link to https://docs.microsoft.com/en-us/rest/api/monitor/filter-syntax
cmd/bosun/expr/azure.go
Outdated
tg = azureIntervalToTimegrain(interval) | ||
} | ||
|
||
// Set azure aggregation method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find various "azure" comments and replace with "Azure" (casing)
cmd/bosun/expr/azure.go
Outdated
if err != nil { | ||
slog.Errorf("failure to parse remaning reads from azure response") | ||
} else { | ||
collect.Sample("azure.remaining_reads", opentsdb.TagSet{"prefix": prefix}, float64(readsRemaining)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment explaining why this is sampled
cmd/bosun/expr/expr.go
Outdated
// collectCache is a helper function for collecting metrics on | ||
// the expression cache | ||
func collectCacheHit(cacheName, qType string, hit bool) { | ||
tags := opentsdb.TagSet{"query_type": qType, "name": cacheName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add metadata for these metrics
cmd/bosun/expr/azure.go
Outdated
} | ||
series := make(Series) | ||
tags := make(opentsdb.TagSet) | ||
tags["rsg"] = rsg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref constant here
cmd/bosun/expr/azure.go
Outdated
} | ||
} | ||
for _, mValue := range *dataContainer.Data { | ||
exValue := azureExtractMetricValue(&mValue, aggLong) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment
cmd/bosun/expr/azure.go
Outdated
for res := range resCh { | ||
queryResults = append(queryResults, res) | ||
} | ||
// Merge the query results into a single seriesSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip merge if length of set is not gt than 1
cmd/bosun/expr/azure.go
Outdated
// or tags associated with that resource | ||
func AzureFilterResources(e *State, T miniprofiler.Timer, resources AzureResources, filter string) (r *Results, err error) { | ||
r = new(Results) | ||
bqf, err := boolq.Parse(filter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment
if len(sp) != 2 { | ||
return false, fmt.Errorf("bad filter, filter must be in k:v format, got %v", filter) | ||
} | ||
key := strings.ToLower(sp[0]) // Make key case insensitive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note case insensitivity in expression docs
cmd/bosun/expr/azure.go
Outdated
key := strings.ToLower(sp[0]) // Make key case insensitive | ||
value := sp[1] | ||
switch key { | ||
case "name": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consts again for name, rsg
cmd/bosun/expr/azure.go
Outdated
if re.MatchString(ar.Name) { | ||
return true, nil | ||
} | ||
case "rsg", "resourcegroup": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove "resourcegroup" alias, no real point, update expr docs to reflect
case string(insights.Maximum): | ||
v = mv.Maximum | ||
case string(insights.Total): | ||
v = mv.Total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add missing None aggregation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm, None isn't a field in the sdk
return string(insights.Maximum), nil | ||
case "total": | ||
return string(insights.Total), nil | ||
case "count": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add missing none aggregation here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty good to me. Just finish out todos and comments.
cmd/bosun/conf/system.go
Outdated
@@ -84,6 +94,7 @@ func (sc *SystemConf) EnabledBackends() EnabledBackends { | |||
b.Influx = sc.InfluxConf.URL != "" | |||
b.Elastic = len(sc.ElasticConf["default"].Hosts) != 0 | |||
b.Annotate = len(sc.AnnotateConf.Hosts) != 0 | |||
b.AzureMonitor = sc.AzureMonitorConf["default"].ClientId != "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a weak thing to check. Are they required to have a "default"? Is ClientID required? (assume thats a credential of some kind?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed will make something better, this is left from my first pass
clients.MetricsClient = insights.NewMetricsClient(conf.SubscriptionId) | ||
clients.MetricDefinitionsClient = insights.NewMetricDefinitionsClient(conf.SubscriptionId) | ||
clients.ResourcesClient = resources.NewClient(conf.SubscriptionId) | ||
if conf.DebugRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these debug things long term? I'd probably prefer not having a million different config options if we can avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two options. Could help with people debugging issues, in particular since I saw what I think is a bug in the SDK about error being missed.
cmd/bosun/conf/system.go
Outdated
if err != nil { | ||
// Should not hit this since we check for authorizer errors in Validation | ||
// This is checked before because this method is not called until the an expression is called | ||
slog.Fatal("Azure conf: ", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fatal feels wrong here. Does this method get invoked every time an expression runs? If azure is down intermittently, could this crash bosun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this actually make any API calls and is just a validity thing. So it should be caught by Valid() method, but added this here in case I'm wrong.
cmd/bosun/conf/system.go
Outdated
return allClients | ||
} | ||
|
||
func azureLogRequest() autorest.PrepareDecorator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these even required long term?
F: AzureMultiQuery, | ||
PrefixEnabled: true, | ||
}, | ||
"azmd": { // TODO Finish and document this func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
F: AzureResourcesByType, | ||
PrefixEnabled: true, | ||
}, | ||
"azrf": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
azrf or azfr? Inner function is "FilterResources", so maybe azfr matches better? I kinda hate short names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think like azrf because it shares the azr prefix with azrt
} | ||
|
||
// AzureMetricDefinitions fetches metric information for a specific resource and metric tuple | ||
// TODO make this return and not fmt.Printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
return | ||
} | ||
} | ||
st := e.now.Add(time.Duration(-sd)).Format(azTimeFmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"startTime" and "endTime" would not be unreasonable names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is done I want to go back and make a method on the State that does since we use it all the time.
7919b3d
to
4dce241
Compare
this includes an incomplete azure metadata function with TODOs to be completed in a future commit.
so metrics can be collected
No description provided.