Azure: AzureMonitorMetrics - change response to be dataplane compliant #69308
Conversation
kylebrandt
added
datasource/Azure
|
Backend code coverage report for PR #69308
|
|
Frontend code coverage report for PR #69308 |
There was a problem hiding this comment.
Thank you for creating this PR @kylebrandt (sorry it took awhile to review). We can merge this in as is and then I'll take a look at making the change for DisplayName 😊
|
Shall we merge this @kylebrandt 😊 |
|
@aangelisc My only hesitation is not having the change under a feature flag (even if it is one on by default, at least it gives a way to go back with downgrading). WDYT? |
|
I guess from my PoV these changes aren't very material (we're just adding some metadata to the frame). What situation could you envision where this may need to be reverted? @kylebrandt |
Consumers (e.g. alerting, viz, recorded queries) may hit different code paths if they see dataplane data (which is indicated by the metadata). They have there own switches, but that would impact other data sources as things move forward. So it is safer I think to be able to pull out the change without a downgrade of grafana. |
aangelisc
requested review from
summerwollin and
dafydd-t
and removed request for
a team
github-actions
bot
added
type/docs
area/frontend
pr/external
|
Sorry this fell through the cracks @kylebrandt, I've created the feature toggle and made use of it. Let me know if this is okay 😊 |
# Conflicts: # packages/grafana-data/src/types/featureToggles.gen.ts # pkg/services/featuremgmt/toggles_gen.csv # pkg/services/featuremgmt/toggles_gen.go # pkg/tsdb/azuremonitor/metrics/azuremonitor-datasource_test.go
reference: https://grafana.github.io/dataplane/
Things generally look good as they are in terms of being compliant with dataplane, so this is a small change. This is a bit of an aside, but I noticed the field
DisplayNameproperty is set, but that is an older field an per the documentationDisplayNameFromDSshould be used instead (https://pkg.go.dev/github.com/grafana/grafana-plugin-sdk-go/data#FieldConfig).The main current impact of this is that SSE checks for this metadata and takes a different code path that should be predictable. So we should wrap this in a feature flag to be safe, and check that Grafana managed alerts behave as expected (in particular in regards to DisplayName).