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

Add metrics as a supported feature in the SupportsFeatureMixin #13381

Closed
wants to merge 1 commit into from
Closed

Add metrics as a supported feature in the SupportsFeatureMixin #13381

wants to merge 1 commit into from

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jan 6, 2017

This adds "metrics" as a supported feature to the list. At a minimum, we will need to use this for the Azure provider as part of the effort towards supporting usgov environments.

https://bugzilla.redhat.com/show_bug.cgi?id=1403366

@miq-bot
Copy link
Member

miq-bot commented Jan 6, 2017

Checked commit https://github.com/djberg96/manageiq/commit/e5c4ae59599306f3a3359f655f0fa178281f3b80 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. ⭐

@durandom
Copy link
Member

before adding new features to the list, I'd like to see how they are being used. Otherwise we risk adding stale features like :events

@djberg96
Copy link
Contributor Author

djberg96 commented Feb 9, 2017

@durandom See ManageIQ/manageiq-providers-azure#36 for how it will be used.

@durandom
Copy link
Member

@djberg96 is it still needed? In your azure PR you now use :timelines

Also, I think a feature should always be refactored / introduced as a whole. Because we have this list #11281 and I dont want false negatives here.

@djberg96
Copy link
Contributor Author

@durandom Does :timelines hook into metrics as well? Or just events?

@durandom
Copy link
Member

I believe it's just events.
Afaik, the metrics are not displayed in the event timelines at all.
Hence my initial refactoring, as timelines just doesnt reveal any background...

@djberg96
Copy link
Contributor Author

@durandom Alright, well, if you don't want to add :metrics, which feature from the mixin do you suggest I hook into in order to tell the ems whether or not metrics are supported?

@durandom
Copy link
Member

@djberg96 oh, sorry, I just looked at you azure PR and saw it doesnt contain the :metrics feature anymore.

@durandom
Copy link
Member

So, I'm not 100% sure what it means to support metrics or not.
I guess it involves at least

  • starting a metrics collector worker
  • displaying them in the UI

@djberg96
Copy link
Contributor Author

@durandom, I assume so. @jrafanie, can you confirm?

@jrafanie
Copy link
Member

@durandom, I assume so. @jrafanie, can you confirm?

I have no idea. 😕
It makes sense to require some way to populate metrics (metrics collector worker) and how to display them... maybe?

@durandom
Copy link
Member

@djberg96 so how about putting this on WIP and you explore in azure what it takes to support :metrics - but with a simple plain old ruby method?

Once this is fleshed out we can refactor this to a proper supports :feature thing. With all providers responding truthfully to it.

@djberg96
Copy link
Contributor Author

@durandom I'm not sure what you mean exactly. To determine if metrics is supported requires the azure-armrest API. It looks like this:

rps = ::Azure::Armrest::ResourceProviderService.new(conf)
rps.get('Microsoft.Insights').registration_state.casecmp('registered').zero?

In other words, if the "Microsoft.Insights" provider is not registered, then metrics is not supported.

@durandom
Copy link
Member

As I understand, metrics is supported conditionally in azure. That seems those with Insights - or whatever.

Now what to do? Check if a worker should be started? Display something in the UI, etc. etc.

If we know, at what places in the code we have to query for supports_metrics - we can roll this out to the supports feature mixin.

But until then, I suggest you do this with a simple method.

@jrafanie
Copy link
Member

Does a provider start a metrics collector by default? If so, that's the part that should be changed to be an attribute of the provider that allows/disallows starting of various workers for that provider. Maybe?

@djberg96
Copy link
Contributor Author

@jrafanie No, you have to opt-in for metrics collection.

I'll go ahead and close this one for now as I don't rely on it.

@djberg96 djberg96 closed this Mar 14, 2017
@moolitayer
Copy link

I think we will now have use for this one: Openshift is going to optionally have metrics collection.

  • when adding the provider a user can select to create it with one or two endpoints (optional one for metrics)
  • metrics workers should only start iff there is a relevant endpoint
  • some screens (ad hock metrics) should optionally not be available

@durandom
Copy link
Member

@moolitayer sure, once you have a POC or another PR that uses it just link it to this PR

@moolitayer
Copy link

@moolitayer sure, once you have a POC or another PR that uses it just link it to this PR

ManageIQ/manageiq-providers-kubernetes#7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants