Add Azure Metrics Integration #2802
Conversation
kgeckhart
force-pushed
the
keckhart/azure-metrics-integration
branch
from
5dedb01
to
aa66670
Compare
kgeckhart
requested review from
karengermond,
rgeyer,
gaantunes,
captncraig and
marctc
as code owners
| ConcurrencySubscription int `long:"concurrency.subscription" env:"CONCURRENCY_SUBSCRIPTION" description:"Concurrent subscription fetches" default:"5"` | ||
| // Limits the number of concurrent metric requests for a single subscription - value taken from OSS exporter | ||
| ConcurrencySubscriptionResource int `long:"concurrency.subscription.resource" env:"CONCURRENCY_SUBSCRIPTION_RESOURCE" description:"Concurrent requests per resource (inside subscription requests)" default:"10"` | ||
| Cache bool `long:"enable-caching" env:"ENABLE_CACHING" description:"Enable internal caching"` |
There was a problem hiding this comment.
the annotation in here come from the azure lib right?
There was a problem hiding this comment.
Yeah the azure lib defines these as embedded structs so to pass it to the prober I have to inherit this. I changing this in the upstream because I wasn't sure how it would impact the config parsing.
| * [Read access to the resources which will be queried by Resource Graph](https://learn.microsoft.com/en-us/azure/governance/resource-graph/overview#permissions-in-azure-resource-graph) | ||
| * Permissions to call the [Microsoft.Insights Metrics API](https://learn.microsoft.com/en-us/rest/api/monitor/metrics/list) which should be the `Microsoft.Insights/Metrics/Read` permission | ||
|
|
||
| ## Configuration options: |
There was a problem hiding this comment.
If you have something at hand, it'd be nice to have in the docs a simple example of how to scrape some Azure metrics, for example, some full config for scraping some blob storage metrics, or azure compute instances
There was a problem hiding this comment.
👍 I'll add some sample configs above the config options
kgeckhart
force-pushed
the
keckhart/azure-metrics-integration
branch
5 times, most recently
from
8b8731c
to
98ad517
Compare
There was a problem hiding this comment.
LGTM! Waiting for @karengermond before merging for the docs feedback
go.mod
Outdated
| @@ -634,3 +648,6 @@ exclude ( | |||
| ) | |||
|
|
|||
| replace github.com/github/smimesign => github.com/grafana/smimesign v0.2.1-0.20220408144937-2a5adf3481d3 | |||
|
|
|||
| // To be used until upstream PR of fixes is merged https://github.com/webdevops/azure-metrics-exporter/pull/41 | |||
| replace github.com/webdevops/azure-metrics-exporter => github.com/kgeckhart/azure-metrics-exporter v0.0.0-20230124174512-16e25d5c8d3b | |||
There was a problem hiding this comment.
nit: move to section starting at 601 where all the exporters replaces are defined?
| // Necessary to match OSS definition | ||
| Prober: struct { | ||
| // Limits the number of subscriptions which can concurrently be sending metric requests - value taken from OSS exporter | ||
| ConcurrencySubscription int `long:"concurrency.subscription" env:"CONCURRENCY_SUBSCRIPTION" description:"Concurrent subscription fetches" default:"5"` |
There was a problem hiding this comment.
Out of curiosity: what are these tags used for?
There was a problem hiding this comment.
Unfortunately, it's necessary to match the OSS struct definition. The Opts definition has a bunch of inline structs including this Prober, the tags allow the OSS one to do CLI/env parsing (https://github.com/kgeckhart/azure-metrics-exporter/blob/16e25d5c8d3bc9a0e684ae0ac2e8d202fc6e3309/config/opts.go#L34-L39). I was hesitant to touch it in my fork since I didn't want to break the config loading.
kgeckhart
force-pushed
the
keckhart/azure-metrics-integration
branch
2 times, most recently
from
7142c9b
to
525308e
Compare
|
@karengermond, do you think it would be possible to get a docs review this week? |
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
docs/sources/configuration/integrations/azure-exporter-config.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Karen Germond <110922559+karengermond@users.noreply.github.com>
kgeckhart
force-pushed
the
keckhart/azure-metrics-integration
branch
from
b680f75
to
069485e
Compare
|
Hey, @marctc I think I've addressed everything let me know if there's anything else needed before merge. |
github-actions
bot
added
the
frozen-due-to-age
PR Description
Introduces an azure metrics integration based on https://github.com/webdevops/azure-metrics-exporter
It's currently based on a personal fork with a PR open for upstream https://github.com/kgeckhart/azure-metrics-exporter/tree/kgeckhart/cleanup-and-remove-unused-code
Which issue(s) this PR fixes
Notes to the Reviewer
This exporter integration is slightly different from others. Due to limitations with the azure metrics API the exporter isn't super useful unless you can specify multiple instances. Since we don't want to force consumers to use
integrations-nextyet I setup the exporter so that it will allow overriding almost all configurable params on the metrics handler.This allows an operational mode described here, https://github.com/grafana/agent/blob/98ad517528987f436ad071147273ef6768d463d8/docs/sources/configuration/integrations/azure-exporter-config.md#multiple-azure-services-in-a-single-config. This should make the implementation a lot more valuable for users who are not on
integrations-next.The piece of the OSS exporter we are using doesn't depend on any global values we are using so this should be safe. I'm planning to run the example config for an extended period of time to be safe.
PR Checklist