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.

Sign up for GitHub

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

Jump to bottom

Fixes #22796 - fact importing telemetry #5296

Merged
merged 1 commit into from
Mar 21, 2018
Merged

Fixes #22796 - fact importing telemetry #5296

merged 1 commit into from
Mar 21, 2018

Conversation

lzap
Copy link
Member

@lzap lzap commented Mar 6, 2018

SSIA

This patch is an example on how to add telemetry.

It removes publication of Rails notifications in the importer, it was
unused and we now have a better way of publishing that into telemetry
stack.

@theforeman-bot
Copy link
Member

Issues: #22796

dLobatog
dLobatog suggested changes Mar 13, 2018
View reviewed changes
Copy link
Member

@dLobatog dLobatog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lzap Thanks, this is much cleane! The failing tests are only failing because in test/models/host_test.rb you need to also mock the class_name_humanized method for the stubbed parsers in setup_host_with_nic_parser and setup_host_with_ipmi_parser.

👍 to merge after that if tests are green

app/models/host/base.rb
end
telemetry_increment_counter(:importer_facts_count_interfaces, changed_count, type: parser.class_name_humanized)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is unexpected in some tests that use a mocked parser, could you check that out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand this one?

@lzap
Copy link
Member Author

lzap commented Mar 13, 2018

Thanks rebased.

houndci-bot
houndci-bot reviewed Mar 13, 2018
View reviewed changes
test/models/host_test.rb Outdated
[host, parser]
end

def setup_host_with_ipmi_parser(ipmi_attributes)
host = FactoryBot.create(:host, :hostgroup => FactoryBot.create(:hostgroup))
hash = ipmi_attributes.with_indifferent_access
primary = host.primary_interface
parser = stub(:ipmi_interface => hash, :interfaces => {}, :suggested_primary_interface => [ primary.identifier, {:macaddress => primary.mac, :ipaddress => primary.ip} ])
parser = stub(:class_name_humanized => 'TestParser', :ipmi_interface => hash, :interfaces => {}, :suggested_primary_interface => [ primary.identifier, {:macaddress => primary.mac, :ipaddress => primary.ip} ])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside square brackets detected.

test/models/host_test.rb Outdated
@@ -1370,7 +1370,7 @@ def teardown

test "#set_interfaces handles no interfaces" do
host = FactoryBot.create(:host, :hostgroup => FactoryBot.create(:hostgroup))
parser = stub(:ipmi_interface => {}, :interfaces => {}, :suggested_primary_interface => [ nil, nil ])
parser = stub(:class_name_humanized => 'TestParser', :ipmi_interface => {}, :interfaces => {}, :suggested_primary_interface => [ nil, nil ])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside square brackets detected.

@dLobatog
Copy link
Member

[test foreman]

I'll fix the hound spacing issues if tests pass

@dLobatog
Copy link
Member

Tests pass, the failure in MySQL is being addressed on https://github.com/theforeman/foreman/pull/5325/files
I'll fix the rubocop warning & merge. Thanks @lzap 🎖️

dLobatog
dLobatog approved these changes Mar 14, 2018
View reviewed changes
Copy link
Member

@dLobatog dLobatog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, I forgot 'develop' is now protected..

remote: error: GH006: Protected branch update failed for refs/heads/develop.
remote: error: At least 1 approving review is required by reviewers with write access.

Please just address the hound issues & I'll merge right away. Thanks !

@san7ket
Copy link

san7ket commented Mar 15, 2018

Can successfully list the facts telemetry

fm_rails_importer_facts_count_interfaces{type="puppet_fact_parser"} 1
# HELP fm_rails_importer_facts_count_processed Number of facts processed (added, updated, deleted) per importer type
# TYPE fm_rails_importer_facts_count_processed counter
fm_rails_importer_facts_count_processed{action="added",type="puppet_fact_name"} 0
fm_rails_importer_facts_count_processed{action="deleted",type="puppet_fact_name"} 11
fm_rails_importer_facts_count_processed{action="updated",type="puppet_fact_name"} 26
# HELP fm_rails_importer_facts_import_duration Duration of fact import (ms) per importer type
# TYPE fm_rails_importer_facts_import_duration histogram
fm_rails_importer_facts_import_duration_bucket{type="",le="10"} 3

@lzap
Copy link
Member Author

lzap commented Mar 19, 2018

Rebased.

@mmoll mmoll merged commit f2ac905 into theforeman:develop Mar 21, 2018
@mmoll
Copy link
Contributor

mmoll commented Mar 21, 2018

merged, díky @lzap!

@lzap lzap deleted the tele-imports-22796 branch March 26, 2018 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@dLobatog dLobatog dLobatog approved these changes

Assignees
No one assigned
Labels
Ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lzap @theforeman-bot @dLobatog @san7ket @mmoll @houndci-bot