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

fix: improve performance of network diagram #614

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,16 @@ The maximum amount of time in seconds to attempt to generate the diff between tw
**YAML configuration key name:** `pact_content_diff_timeout`<br/>
**Default:** `15`<br/>

### network_diagram_max_pacticipants

The maximum number of pacticipants to include in the network diagram. When too many pacticipants are included, the diagram becomes unreadable,
and at large numbers, the graph will not render due to browser performance issues.

**Environment variable name:** `PACT_BROKER_NETWORK_DIAGRAM_MAX_PACTICIPANTS`<br/>
**YAML configuration key name:** `network_diagram_max_pacticipants`<br/>
**Default:** `150`<br/>
**Allowed values:** A positive integer<br/>

<br/>

## Miscellaneous
Expand Down
6 changes: 6 additions & 0 deletions docs/configuration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,12 @@ groups:
The maximum amount of time in seconds to attempt to generate the diff between two pacts before aborting the request. This is required due to performance issues in the underlying diff generation code.
default_value: 15
supported_versions: From 2.99.0
network_diagram_max_pacticipants:
description: |-
The maximum number of pacticipants to include in the network diagram. When too many pacticipants are included, the diagram becomes unreadable,
and at large numbers, the graph will not render due to browser performance issues.
default_value: 150
allowed_values_description: A positive integer
- title: Miscellaneous
vars:
features:
Expand Down
13 changes: 11 additions & 2 deletions lib/pact_broker/api/resources/group.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
require "pact_broker/string_refinements"
require "pact_broker/api/resources/base_resource"
require "pact_broker/api/decorators/relationships_csv_decorator"

module PactBroker
module Api
module Resources
class Group < BaseResource
using PactBroker::StringRefinements

def content_types_provided
[["text/csv", :to_csv]]
end
Expand Down Expand Up @@ -32,9 +35,15 @@ def policy_name
private

def group
@group ||= group_service.find_group_containing(pacticipant)
@group ||= group_service.find_group_containing(pacticipant, max_pacticipants: max_pacticipants)
end

def max_pacticipants
if request.query["maxPacticipants"]&.integer?
request.query["maxPacticipants"].to_i
end
end
end
end
end
end
end
6 changes: 5 additions & 1 deletion lib/pact_broker/config/runtime_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class RuntimeConfiguration < Anyway::Config
allow_dangerous_contract_modification: false,
semver_formats: ["%M.%m.%p%s%d", "%M.%m", "%M"],
seed_example_data: true,
network_diagram_max_pacticipants: 150,
features: {}
)

Expand All @@ -107,7 +108,10 @@ def self.getter_and_setter_method_names
config_attributes + config_attributes.collect{ |k| "#{k}=".to_sym } + extra_methods - [:base_url]
end

coerce_types(features: COERCE_FEATURES)
coerce_types(
features: COERCE_FEATURES,
network_diagram_max_pacticipants: :integer
)
sensitive_values(:database_url, :database_password)

def log_level= log_level
Expand Down
43 changes: 38 additions & 5 deletions lib/pact_broker/groups/service.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require "pact_broker/repositories"
require "pact_broker/relationships/groupify"
require "pact_broker/domain/index_item"

module PactBroker
module Groups
Expand All @@ -8,13 +8,46 @@ module Service
extend PactBroker::Repositories
extend PactBroker::Services

def find_group_containing pacticipant
groups.find { | group | group.include_pacticipant? pacticipant }
# Returns a list of all the integrations (PactBroker::Domain::IndexItem) that are connected to the given pacticipant.
# @param pacticipant [PactBroker::Domain::Pacticipant] the pacticipant for which to return the connected pacticipants
# @option max_pacticipants [Integer] the maximum number of pacticipants to return, or nil for no maximum. 40 is about the most applications you can meaningfully show in the circle network diagram.
# @return [PactBroker::Domain::Group]
def find_group_containing(pacticipant, max_pacticipants: nil)
PactBroker::Domain::Group.new(build_index_items(integrations_connected_to(pacticipant, max_pacticipants)))
end

def groups
Relationships::Groupify.call(index_service.find_all_index_items)
def integrations_connected_to(pacticipant, max_pacticipants)
PactBroker::Integrations::Integration
.eager(:consumer, :provider)
.where(id: ids_of_integrations_connected_to(pacticipant, max_pacticipants))
.all
end
private_class_method :integrations_connected_to

def build_index_items(integrations)
integrations.collect do | integration |
PactBroker::Domain::IndexItem.new(integration.consumer, integration.provider)
end
end
private_class_method :build_index_items

def ids_of_integrations_connected_to(pacticipant, max_pacticipants)
integrations = []
connected_pacticipants = Set.new([pacticipant.id])
new_connected_pacticipants = Set.new([pacticipant.id])

loop do
new_integrations = PactBroker::Integrations::Integration.including_pacticipant_id(new_connected_pacticipants.to_a).exclude(id: integrations.collect(&:id)).all
integrations.concat(new_integrations)
pacticipant_ids_for_new_integrations = Set.new(new_integrations.flat_map(&:pacticipant_ids))
new_connected_pacticipants = pacticipant_ids_for_new_integrations - connected_pacticipants
connected_pacticipants.merge(pacticipant_ids_for_new_integrations)
break if new_connected_pacticipants.empty? || (max_pacticipants && connected_pacticipants.size >= max_pacticipants)
end

integrations.collect(&:id).uniq
end
private_class_method :ids_of_integrations_connected_to
end
end
end
10 changes: 10 additions & 0 deletions lib/pact_broker/integrations/integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ class Integration < Sequel::Model(Sequel::Model.db[:integrations].select(:id, :c
end
end)

dataset_module do
def including_pacticipant_id(pacticipant_id)
where(consumer_id: pacticipant_id).or(provider_id: pacticipant_id)
end
end

def self.compare_by_last_action_date a, b
if b.latest_pact_or_verification_publication_date && a.latest_pact_or_verification_publication_date
b.latest_pact_or_verification_publication_date <=> a.latest_pact_or_verification_publication_date
Expand Down Expand Up @@ -111,6 +117,10 @@ def consumer_name
def provider_name
provider.name
end

def pacticipant_ids
[consumer_id, provider_id]
end
end
end
end
Expand Down
45 changes: 0 additions & 45 deletions lib/pact_broker/relationships/groupify.rb

This file was deleted.

4 changes: 4 additions & 0 deletions lib/pact_broker/string_refinements.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ def blank?
end

refine String do
def integer?
self =~ /^\d+$/
end

def present?
!blank?
end
Expand Down
3 changes: 2 additions & 1 deletion lib/pact_broker/ui/controllers/groups.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@ def locals(overrides)
pacticipant = pacticipant_service.find_pacticipant_by_name(params[:name])
{
csv_path: "#{base_url}/groups/#{ERB::Util.url_encode(params[:name])}.csv",
max_pacticipants: PactBroker.configuration.network_diagram_max_pacticipants,
pacticipant_name: params[:name],
repository_url: pacticipant&.repository_url,
base_url: base_url,
pacticipant: pacticipant,
details_url: "#{base_url}/pacticipants/#{ERB::Util.url_encode(params[:name])}",
network_url: "#{base_url}/pacticipants/#{ERB::Util.url_encode(params[:name])}/network"
network_url: "#{base_url}/pacticipants/#{ERB::Util.url_encode(params[:name])}/network?maxPacticipants=#{PactBroker.configuration.network_diagram_max_pacticipants}"
}.merge(overrides)
end
end
Expand Down
3 changes: 2 additions & 1 deletion lib/pact_broker/ui/views/groups/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,9 @@ window.onload = function() {
.attr("d","M 10 0 L 10 10 L 0 5 z")
.attr("fill", "#A0A0A0");

const maxPacticipants = new URL(location).searchParams.get("maxPacticipants") || <%= max_pacticipants %>;

d3.text("<%= csv_path %>", "text/csv", function(unparsedData) {
d3.text(`<%= csv_path %>?maxPacticipants=${maxPacticipants}`, "text/csv", function(unparsedData) {
var data=d3.csv.parseRows(unparsedData);
pacticipants = parseCSV(data);
pacticipantNameArray = getPacticipantNames(pacticipants);
Expand Down
13 changes: 11 additions & 2 deletions spec/lib/pact_broker/api/resources/group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module Resources
allow(decorator).to receive(:to_csv).and_return(csv)
end

subject { get "/groups/Some%20Service", "", {"HTTP_X_My_App_Version" => "2"} }
subject { get "/groups/Some%20Service", "", { "HTTP_X_My_App_Version" => "2" } }

context "when the pacticipant exists" do

Expand All @@ -33,7 +33,7 @@ module Resources
end

it "finds the group containing the pacticipant" do
expect(PactBroker::Groups::Service).to receive(:find_group_containing).with(pacticipant)
expect(PactBroker::Groups::Service).to receive(:find_group_containing).with(pacticipant, max_pacticipants: nil)
subject
end

Expand All @@ -56,6 +56,15 @@ module Resources
subject
expect(last_response.body).to eq csv
end

context "when maxPacticipants is specified" do
subject { get "/groups/Some%20Service", { "maxPacticipants" => "30" }, { "HTTP_X_My_App_Version" => "2" } }

it "finds the group containing the pacticipant" do
expect(PactBroker::Groups::Service).to receive(:find_group_containing).with(pacticipant, max_pacticipants: 30)
subject
end
end
end

context "when the pacticipant does not exist" do
Expand Down
67 changes: 36 additions & 31 deletions spec/lib/pact_broker/groups/service_spec.rb
Original file line number Diff line number Diff line change
@@ -1,52 +1,57 @@
require "spec_helper"
require "pact_broker/groups/service"
require "pact_broker/index/service"

module PactBroker

module Groups
describe Service do

describe "#find_group_containing" do
before do
td.create_consumer("app a")
.create_provider("app x")
.create_integration
.create_consumer("app b")
.create_provider("app y")
.create_integration
.use_consumer("app y")
.create_provider("app z")
.create_integration
.use_consumer("app z")
.use_provider("app y")
.create_integration
end

let(:consumer_a) { double("consumer a", name: "consumer a", id: 1)}
let(:consumer_b) { double("consumer b", name: "consumer b", id: 2)}
let(:app_a) { td.find_pacticipant("app a") }
let(:app_b) { td.find_pacticipant("app b") }

let(:provider_x) { double("provider x", name: "provider x", id: 3)}
let(:provider_y) { double("provider y", name: "provider y", id: 4)}
let(:app_x) { td.find_pacticipant("app x") }
let(:app_y) { td.find_pacticipant("app y") }
let(:app_z) { td.find_pacticipant("app z") }

let(:relationship_1) { Domain::IndexItem.new(consumer_a, provider_x) }
let(:relationship_2) { Domain::IndexItem.new(consumer_b, provider_y) }
let(:relationship_1) { Domain::IndexItem.new(app_a, app_x) }
let(:relationship_2) { Domain::IndexItem.new(app_b, app_y) }
let(:relationship_3) { Domain::IndexItem.new(app_y, app_z) }
let(:relationship_3) { Domain::IndexItem.new(app_z, app_y) }

let(:group_1) { Domain::Group.new(relationship_1) }
let(:group_2) { Domain::Group.new(relationship_2) }
let(:group_2) { Domain::Group.new(relationship_2, relationship_3) }

let(:relationship_list) { double("relationship list") }
let(:groups) { [group_1, group_2]}
subject { Service.find_group_containing(app_b) }

subject { Service.find_group_containing(consumer_b) }

before do
allow(PactBroker::Index::Service).to receive(:find_index_items).and_return(relationship_list)
allow(Relationships::Groupify).to receive(:call).and_return(groups)
end

it "retrieves a list of the relationships" do
allow(Index::Service).to receive(:find_index_items)
subject
it "returns the Group containing the given pacticipant" do
expect(subject.size).to eq 3
expect(subject).to include(have_attributes(consumer_name: "app b", provider_name: "app y"))
expect(subject).to include(have_attributes(consumer_name: "app y", provider_name: "app z"))
expect(subject).to include(have_attributes(consumer_name: "app z", provider_name: "app y"))
end

it "turns the relationships into groups" do
expect(Relationships::Groupify).to receive(:call).with(relationship_list)
subject
end
context "when a max_pacticipants is specified" do
subject { Service.find_group_containing(app_b, max_pacticipants: 2) }

it "returns the Group containing the given pacticipant" do
expect(subject).to be group_2
it "stops searching before reaching the end of the group" do
expect(subject.size).to eq 1
expect(subject).to include(have_attributes(consumer_name: "app b", provider_name: "app y"))
end
end

end

end
end
end
Loading