From 2c011cb5b452409db7fe1c810f1ad7440a6cedce Mon Sep 17 00:00:00 2001 From: charlieablett Date: Thu, 2 May 2019 12:16:49 +1200 Subject: Implement logger analyzer - Modify GraphqlLogger to subclass JsonLogger - Replace the single-line analyser with one that can log all the GraphQL query related information in one place. - Implement analyzer behavior with spec --- spec/requests/api/graphql_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'spec/requests/api/graphql_spec.rb') diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index cca87c16f27..103b02ba7a7 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -16,6 +16,17 @@ describe 'GraphQL' do end end + context 'logging' do + it 'logs the query' do + expected = { query_string: query, variables: {}, duration: anything } + + expect(Gitlab::GraphqlLogger).to receive(:info).with(expected) + + post_graphql(query) + end + + end + context 'invalid variables' do it 'returns an error' do post_graphql(query, variables: "This is not JSON") -- cgit v1.2.1 From 2a1006416748950805294793f1bc8d6fa7435eea Mon Sep 17 00:00:00 2001 From: charlieablett Date: Thu, 2 May 2019 19:09:10 +1200 Subject: Restructure complexity analyzer Remove instance variables for class re-use, test individual methods, use `monotonic_time` --- spec/requests/api/graphql_spec.rb | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) (limited to 'spec/requests/api/graphql_spec.rb') diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 103b02ba7a7..036dfa41952 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -17,14 +17,32 @@ describe 'GraphQL' do end context 'logging' do - it 'logs the query' do - expected = { query_string: query, variables: {}, duration: anything } - + before do expect(Gitlab::GraphqlLogger).to receive(:info).with(expected) + end - post_graphql(query) + context 'with no variables' do + let(:expected) do + { query_string: query, variables: {}, duration: anything, depth: 0, complexity: 0 } + end + + it 'logs the query' do + post_graphql(query) + end end + context 'with variables' do + let!(:variables) do + { foo: "bar" } + end + let(:expected) do + { query_string: query, variables: variables, duration: anything, depth: 0, complexity: 0 } + end + + it 'logs the query' do + post_graphql(query, variables: variables) + end + end end context 'invalid variables' do -- cgit v1.2.1 From 184a5120dc764d33cece108fbc85b0ec96f7c050 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 22 May 2019 17:13:06 +1200 Subject: Call analyzers from LoggerAnalyzer - Add changelog file - Fix failing tests --- spec/requests/api/graphql_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'spec/requests/api/graphql_spec.rb') diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 036dfa41952..ebf223127b5 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -23,7 +23,7 @@ describe 'GraphQL' do context 'with no variables' do let(:expected) do - { query_string: query, variables: {}, duration: anything, depth: 0, complexity: 0 } + { query_string: query, variables: {}, duration: anything, depth: 1, complexity: 1 } end it 'logs the query' do @@ -33,10 +33,10 @@ describe 'GraphQL' do context 'with variables' do let!(:variables) do - { foo: "bar" } + { "foo" => "bar" } end let(:expected) do - { query_string: query, variables: variables, duration: anything, depth: 0, complexity: 0 } + { query_string: query, variables: variables, duration: anything, depth: 1, complexity: 1 } end it 'logs the query' do -- cgit v1.2.1 From 5f0c230a18b677bd4ec6a4a54085775b0c69a498 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Fri, 24 May 2019 09:29:19 +1200 Subject: Move complexity/depth to `final_value` Tidy tests according to reviewer comments. Move complexity and depth calls from `initial_value` to `final_value` Log variables as json --- spec/requests/api/graphql_spec.rb | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) (limited to 'spec/requests/api/graphql_spec.rb') diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index ebf223127b5..abc24fc0fe8 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -17,31 +17,32 @@ describe 'GraphQL' do end context 'logging' do + shared_examples 'logging a graphql query' do + let(:expected_params) do + { query_string: query, variables: variables.to_json, duration: anything, depth: 1, complexity: 1 } + end + + it 'logs a query with the expected params' do + post_graphql(query, variables: variables) + end + end + before do - expect(Gitlab::GraphqlLogger).to receive(:info).with(expected) + expect(Gitlab::GraphqlLogger).to receive(:info).with(expected_params).once end context 'with no variables' do - let(:expected) do - { query_string: query, variables: {}, duration: anything, depth: 1, complexity: 1 } - end + let(:variables) { {} } - it 'logs the query' do - post_graphql(query) - end + it_behaves_like 'logging a graphql query' end context 'with variables' do - let!(:variables) do + let(:variables) do { "foo" => "bar" } end - let(:expected) do - { query_string: query, variables: variables, duration: anything, depth: 1, complexity: 1 } - end - it 'logs the query' do - post_graphql(query, variables: variables) - end + it_behaves_like 'logging a graphql query' end end -- cgit v1.2.1 From 699532232ca27e6079c553261e0ab1d17317472a Mon Sep 17 00:00:00 2001 From: charlie ablett Date: Thu, 23 May 2019 22:43:47 +0000 Subject: Apply reviewer feedback - Comply doc with guidelines - Improve tests for readability and completeness - Separate out phases visually with newlines - Add `format_message` test - test readability - code and test structure/styling - static query analyzers - call `as_json` on `provided_variables` - add exception handling --- spec/requests/api/graphql_spec.rb | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) (limited to 'spec/requests/api/graphql_spec.rb') diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index abc24fc0fe8..656d6f8b50b 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -19,16 +19,21 @@ describe 'GraphQL' do context 'logging' do shared_examples 'logging a graphql query' do let(:expected_params) do - { query_string: query, variables: variables.to_json, duration: anything, depth: 1, complexity: 1 } + { query_string: query, variables: variables.to_s, duration: anything, depth: 1, complexity: 1 } end it 'logs a query with the expected params' do + expect(Gitlab::GraphqlLogger).to receive(:info).with(expected_params).once + post_graphql(query, variables: variables) end - end - before do - expect(Gitlab::GraphqlLogger).to receive(:info).with(expected_params).once + it 'does not instantiate any query analyzers' do # they are static and re-used + expect(GraphQL::Analysis::QueryComplexity).not_to receive(:new) + expect(GraphQL::Analysis::QueryDepth).not_to receive(:new) + + 2.times { post_graphql(query, variables: variables) } + end end context 'with no variables' do @@ -44,6 +49,19 @@ describe 'GraphQL' do it_behaves_like 'logging a graphql query' end + + context 'when there is an error in the logger' do + before do + allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer).to receive(:process_variables).and_raise(StandardError.new("oh noes!")) + end + + it 'logs the exception in Sentry and continues with the request' do + expect(Gitlab::Sentry).to receive(:track_exception).at_least(1).times + expect(Gitlab::GraphqlLogger).to receive(:info) + + post_graphql(query, variables: {}) + end + end end context 'invalid variables' do -- cgit v1.2.1