signalfx / signalfx-go

Go client library and instrumentation bindings for SignalFx
https://www.signalfx.com
Apache License 2.0
14 stars 48 forks source link

add missing azure services #123

Closed xp-1000 closed 3 years ago

xp-1000 commented 3 years ago

hello

Honestly, I don't understand why the azure integration does not allow to configure all services like other cloud integrations (gcp, aws) do especially since the underlying API accept it:

{"appId":null,"azureEnvironment":"AZURE","created":1613489793389,"creator":"xxx","enabled":true,"id":"xxx","lastUpdated":1613489793389,"lastUpdatedBy":"xxxEAE","name":"test-azure","namedToken":"test-azure","pollRate":300000,"secretKey":null,"services":[],"subscriptions":["yyy"],"syncGuestOsNamespaces":false,"tenantId":"zzz","type":"Azure"}

this example payload passing empty services array [] will enable all services in contrast of specifying a list of services which will only select those obviously.

Digging in git history I understand that it is a limitation from the terraform provider which make the services argument mandatory with at least one element: https://github.com/splunk-terraform/terraform-provider-signalfx/commit/0353e122cfa682e9d49600b9a512cd6802210b0f#diff-aa526560492dd3432307ab965443674778bce1ce10e7c452f41975599b2716f6

However it seems to be a desired breaking change as changelog indicates: https://github.com/hashicorp/terraform-provider-signalfx/blob/master/CHANGELOG.md#4190-april-13-2020

Even if I would prefer to have the ability to configure all services easily without to know them like for aws and gcp there was probably a good reason to that but while I don't know/understand it I prefer to avoid any undesired side affect.

So I updated the azure services list to, at least, be able to configure all of them from the datasource given that it seems to be the only way even if it is not really future proof and will probably show similar problem when you will add new services in your integration.

I tested the terraform provider with this change and it works fine:

      ~ services                 = [
          + "microsoft.eventgrid/domains",
          + "microsoft.eventgrid/eventsubscriptions",
          + "microsoft.eventgrid/extensiontopics",
          + "microsoft.eventgrid/systemtopics",
          + "microsoft.eventgrid/topics",
          + "microsoft.maps/accounts",
          + "microsoft.network/azurefirewalls",
          + "microsoft.network/frontdoors",
          + "microsoft.network/networkinterfaces",
            # (59 unchanged elements hidden)
        ]

I am not sure how should I process to update the provider, should I wait for a tag here and create a Pull Request to bump the lib version used in go.mod here : https://github.com/splunk-terraform/terraform-provider-signalfx ? or simply create an issue ?

thanks

xp-1000 commented 3 years ago

cc @keitwb and @jrcamp (may be should you add relevant maintainers at Signalfx in default reviewers ?)

keitwb commented 3 years ago

Hi @xp-1000, I've asked somebody who is more familiar with the Azure integration API to take a look.

keitwb commented 3 years ago

Ok, so yes I'm not entirely sure why the provider requires at least one but for now just make a PR in the terraform repo to update this repo in go.mod. I made a tag v1.7.17 here to use. Being explicit with the service list in the provider I think is acceptable for now as it reduces uncertainty around what should be monitored or not.

xp-1000 commented 3 years ago

thanks @keitwb

Being explicit with the service list in the provider I think is acceptable for now as it reduces uncertainty around what should be monitored or not.

I agree even if it would be better if someone from Signalfx can update the terraform provider when you add new Azure service to your integration.

In my knowledge, these new services were not announced (e.g. in the "What is new" webui) so it is difficult to trigger the change from our side.