-
Notifications
You must be signed in to change notification settings - Fork 900
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
Legacy metrics capture -> ActiveMetrics adapter #15710
Conversation
47e730b
to
fa75c6b
Compare
I don't see a reason we need to do this, and it actually breaks the tests in some way as that's not the format of the original implementation.
Because Rubocop hates nice things.
69f99b2
to
b93e411
Compare
Ready to roll, @blomquisg @Fryguy |
@@ -0,0 +1 @@ | |||
ActiveMetrics::Base.establish_connection(:adapter => "miq_postgres", :database => "metrics_spike") |
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.
If this gets merged the database will need to change...I'm guessing this would end up being Rails.database_configuration.database
(or however you get 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.
Oh I guess it doesn't really matter, but if we switch adapters, I'm thinking the database name should be consistent.
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.
O_o Good catch, it absolutely matters does it not? This should absolutely fall on its face without a change.
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.
Oh yeah, forgot the adapter here just steals AR::Base's, nevermind.
{}.tap do |index| | ||
metrics.each do |m| | ||
interval_name = m.fetch_path(:tags, :capture_interval_name) | ||
resource = m[:resource] || m.fetch_path(:tags, :resource_type).safe_constantize.find(m.fetch_path(:tags, :resource_id)) |
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.
Interesting...I didn't expect the resource_type to be in a tag. I thought for the other adapters, resource_type and resource_id were top-level keys.
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.
They're top level in the metric (a realtime row) but are written as 'tags' in each respective adapter. You had said you took that from Influx, where tags are indexed (values are not). Unsure if it really makes sense to differentiate, but we can change that later.
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.
Probably doesn't make much of a difference for now because the caller is sending :resource anyway
@chrisarcand Where do the bottlenecks happen? I remember in my initial spike on this, I had brought that over (even though it shouldn't be brought over), but I don't see it here at all, which was surprising. |
@Fryguy Bottlenecks? I'm not sure what you're referring to. |
Checked commits chrisarcand/manageiq@8698abe~...3c5ae5b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 lib/active_metrics/connection_adapters/miq_postgres_adapter.rb
|
@chrisarcand I'm referring to the Bottleneck calculations. However, the reason I don't see it is because it lives in rollups : manageiq/app/models/metric/ci_mixin/rollup.rb Line 104 in 280e5ad
|
This moves the current write interface in metrics processing to an ActiveMetrics adapter pattern.
Presumably one can now write an adapter for a different backend such as Elasticsearch that conforms to this interface with minimal hair pulling.