diff --git a/.rubocop.yml b/.rubocop.yml index acbd92bdecb5d387b39238cbc60115a1399d1e75..0d94cafedbce97a8e464f2343ca5407a766250e3 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -146,6 +146,7 @@ Naming/FileName: - XMPP - XSRF - XSS + - GRPC # GitLab ################################################################### diff --git a/app/views/groups/labels/index.html.haml b/app/views/groups/labels/index.html.haml index 6ba9c578eeb904863c5562a5b04b0cd7397ffb7f..697d489db99f0e22cb8f883c9a79cc8307fab24b 100644 --- a/app/views/groups/labels/index.html.haml +++ b/app/views/groups/labels/index.html.haml @@ -5,15 +5,10 @@ - subscribed = params[:subscribed] - labels_or_filters = @labels.exists? || search.present? || subscribed.present? -- if @labels.present? && can_admin_label - - content_for(:header_content) do - .nav-controls - = link_to _('New label'), new_group_label_path(@group), class: "btn btn-success" - - if labels_or_filters #promote-label-modal %div{ class: container_class } - = render 'shared/labels/nav' + = render 'shared/labels/nav', labels_or_filters: labels_or_filters, can_admin_label: can_admin_label .labels-container.prepend-top-5 - if @labels.any? diff --git a/app/views/projects/labels/index.html.haml b/app/views/projects/labels/index.html.haml index 56b06374d6d53c20d12834aefe68ba347cca6adc..bb7c297ba1ffb5e573d34c8df36a48cd5dc509d9 100644 --- a/app/views/projects/labels/index.html.haml +++ b/app/views/projects/labels/index.html.haml @@ -5,15 +5,10 @@ - subscribed = params[:subscribed] - labels_or_filters = @labels.exists? || @prioritized_labels.exists? || search.present? || subscribed.present? -- if labels_or_filters && can_admin_label - - content_for(:header_content) do - .nav-controls - = link_to _('New label'), new_project_label_path(@project), class: "btn btn-success qa-label-create-new" - - if labels_or_filters #promote-label-modal %div{ class: container_class } - = render 'shared/labels/nav' + = render 'shared/labels/nav', labels_or_filters: labels_or_filters, can_admin_label: can_admin_label .labels-container.prepend-top-10 - if can_admin_label diff --git a/app/views/shared/labels/_nav.html.haml b/app/views/shared/labels/_nav.html.haml index 98572db738bf887b8719a6837bc83aceeda3feaa..e69246dd0eb5f31e8392817208bacbd8feece758 100644 --- a/app/views/shared/labels/_nav.html.haml +++ b/app/views/shared/labels/_nav.html.haml @@ -18,3 +18,7 @@ %button.btn.btn-default{ type: "submit", "aria-label" => _('Submit search') } = icon("search") = render 'shared/labels/sort_dropdown' + - if labels_or_filters && can_admin_label && @project + = link_to _('New label'), new_project_label_path(@project), class: "btn btn-success qa-label-create-new" + - if labels_or_filters && can_admin_label && @group + = link_to _('New label'), new_group_label_path(@group), class: "btn btn-success qa-label-create-new" diff --git a/changelogs/unreleased/an-opentracing-propagation.yml b/changelogs/unreleased/an-opentracing-propagation.yml new file mode 100644 index 0000000000000000000000000000000000000000..d9aa7cd0048c18b0b9b54cbc1cde97de1cbed66f --- /dev/null +++ b/changelogs/unreleased/an-opentracing-propagation.yml @@ -0,0 +1,5 @@ +--- +title: Adds inter-service OpenTracing propagation +merge_request: 24239 +author: +type: other diff --git a/changelogs/unreleased/fix-56558-move-primary-button.yml b/changelogs/unreleased/fix-56558-move-primary-button.yml new file mode 100644 index 0000000000000000000000000000000000000000..4dcc896b32788b8ed00f217f281ebd1ba20ab8f1 --- /dev/null +++ b/changelogs/unreleased/fix-56558-move-primary-button.yml @@ -0,0 +1,5 @@ +--- +title: Moved primary button for labels to follow the design patterns used on rest of the site +merge_request: +author: Martin Hobert +type: fixed diff --git a/config/initializers/tracing.rb b/config/initializers/tracing.rb index be95f30d075386627069179eec5d13007ece1db3..46e8125daf8051352d1710ffe937ae5f7123633d 100644 --- a/config/initializers/tracing.rb +++ b/config/initializers/tracing.rb @@ -3,6 +3,26 @@ if Gitlab::Tracing.enabled? require 'opentracing' + Rails.application.configure do |config| + config.middleware.insert_after Gitlab::Middleware::CorrelationId, ::Gitlab::Tracing::RackMiddleware + end + + # Instrument the Sidekiq client + Sidekiq.configure_client do |config| + config.client_middleware do |chain| + chain.add Gitlab::Tracing::Sidekiq::ClientMiddleware + end + end + + # Instrument Sidekiq server calls when running Sidekiq server + if Sidekiq.server? + Sidekiq.configure_server do |config| + config.server_middleware do |chain| + chain.add Gitlab::Tracing::Sidekiq::ServerMiddleware + end + end + end + # In multi-processed clustered architectures (puma, unicorn) don't # start tracing until the worker processes are spawned. This works # around issues when the opentracing implementation spawns threads diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 8bf8a3b53cd02cbe2bd6d0c3205e576cf6e4466e..85afbd85fe62f06688a305882581236ec2cd90cc 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -52,11 +52,18 @@ module Gitlab klass = stub_class(name) addr = stub_address(storage) creds = stub_creds(storage) - klass.new(addr, creds) + klass.new(addr, creds, interceptors: interceptors) end end end + def self.interceptors + return [] unless Gitlab::Tracing.enabled? + + [Gitlab::Tracing::GRPCInterceptor.instance] + end + private_class_method :interceptors + def self.stub_cert_paths cert_paths = Dir["#{OpenSSL::X509::DEFAULT_CERT_DIR}/*"] cert_paths << OpenSSL::X509::DEFAULT_CERT_FILE if File.exist? OpenSSL::X509::DEFAULT_CERT_FILE diff --git a/lib/gitlab/tracing/common.rb b/lib/gitlab/tracing/common.rb new file mode 100644 index 0000000000000000000000000000000000000000..5e2b12e3f9083af7e3e55c93237d7b2ddac50add --- /dev/null +++ b/lib/gitlab/tracing/common.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module Gitlab + module Tracing + module Common + def tracer + OpenTracing.global_tracer + end + + # Convience method for running a block with a span + def in_tracing_span(operation_name:, tags:, child_of: nil) + scope = tracer.start_active_span( + operation_name, + child_of: child_of, + tags: tags + ) + span = scope.span + + # Add correlation details to the span if we have them + correlation_id = Gitlab::CorrelationId.current_id + if correlation_id + span.set_tag('correlation_id', correlation_id) + end + + begin + yield span + rescue => e + log_exception_on_span(span, e) + raise e + ensure + scope.close + end + end + + def log_exception_on_span(span, exception) + span.set_tag('error', true) + span.log_kv(kv_tags_for_exception(exception)) + end + + def kv_tags_for_exception(exception) + case exception + when Exception + { + 'event': 'error', + 'error.kind': exception.class.to_s, + 'message': Gitlab::UrlSanitizer.sanitize(exception.message), + 'stack': exception.backtrace.join("\n") + } + else + { + 'event': 'error', + 'error.kind': exception.class.to_s, + 'error.object': Gitlab::UrlSanitizer.sanitize(exception.to_s) + } + end + end + end + end +end diff --git a/lib/gitlab/tracing/grpc_interceptor.rb b/lib/gitlab/tracing/grpc_interceptor.rb new file mode 100644 index 0000000000000000000000000000000000000000..6c2aab73125f77bc25e97beb8569245f51fa21b7 --- /dev/null +++ b/lib/gitlab/tracing/grpc_interceptor.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'opentracing' +require 'grpc' + +module Gitlab + module Tracing + class GRPCInterceptor < GRPC::ClientInterceptor + include Common + include Singleton + + def request_response(request:, call:, method:, metadata:) + wrap_with_tracing(method, 'unary', metadata) do + yield + end + end + + def client_streamer(requests:, call:, method:, metadata:) + wrap_with_tracing(method, 'client_stream', metadata) do + yield + end + end + + def server_streamer(request:, call:, method:, metadata:) + wrap_with_tracing(method, 'server_stream', metadata) do + yield + end + end + + def bidi_streamer(requests:, call:, method:, metadata:) + wrap_with_tracing(method, 'bidi_stream', metadata) do + yield + end + end + + private + + def wrap_with_tracing(method, grpc_type, metadata) + tags = { + 'component' => 'grpc', + 'span.kind' => 'client', + 'grpc.method' => method, + 'grpc.type' => grpc_type + } + + in_tracing_span(operation_name: "grpc:#{method}", tags: tags) do |span| + OpenTracing.inject(span.context, OpenTracing::FORMAT_TEXT_MAP, metadata) + + yield + end + end + end + end +end diff --git a/lib/gitlab/tracing/rack_middleware.rb b/lib/gitlab/tracing/rack_middleware.rb new file mode 100644 index 0000000000000000000000000000000000000000..e6a31293f7ba390a0c77e9893d735bcadff806c7 --- /dev/null +++ b/lib/gitlab/tracing/rack_middleware.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'opentracing' + +module Gitlab + module Tracing + class RackMiddleware + include Common + + REQUEST_METHOD = 'REQUEST_METHOD' + + def initialize(app) + @app = app + end + + def call(env) + method = env[REQUEST_METHOD] + + context = tracer.extract(OpenTracing::FORMAT_RACK, env) + tags = { + 'component' => 'rack', + 'span.kind' => 'server', + 'http.method' => method, + 'http.url' => self.class.build_sanitized_url_from_env(env) + } + + in_tracing_span(operation_name: "http:#{method}", child_of: context, tags: tags) do |span| + @app.call(env).tap do |status_code, _headers, _body| + span.set_tag('http.status_code', status_code) + end + end + end + + # Generate a sanitized (safe) request URL from the rack environment + def self.build_sanitized_url_from_env(env) + request = ActionDispatch::Request.new(env) + + original_url = request.original_url + uri = URI.parse(original_url) + uri.query = request.filtered_parameters.to_query if uri.query.present? + + uri.to_s + end + end + end +end diff --git a/lib/gitlab/tracing/sidekiq/client_middleware.rb b/lib/gitlab/tracing/sidekiq/client_middleware.rb new file mode 100644 index 0000000000000000000000000000000000000000..2b71c1ea21e73409590932ecc3155e282177d19b --- /dev/null +++ b/lib/gitlab/tracing/sidekiq/client_middleware.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'opentracing' + +module Gitlab + module Tracing + module Sidekiq + class ClientMiddleware + include SidekiqCommon + + SPAN_KIND = 'client' + + def call(worker_class, job, queue, redis_pool) + in_tracing_span( + operation_name: "sidekiq:#{job['class']}", + tags: tags_from_job(job, SPAN_KIND)) do |span| + # Inject the details directly into the job + tracer.inject(span.context, OpenTracing::FORMAT_TEXT_MAP, job) + + yield + end + end + end + end + end +end diff --git a/lib/gitlab/tracing/sidekiq/server_middleware.rb b/lib/gitlab/tracing/sidekiq/server_middleware.rb new file mode 100644 index 0000000000000000000000000000000000000000..5b43c4310e622fc9032c28f5dd675ffa8972c0c8 --- /dev/null +++ b/lib/gitlab/tracing/sidekiq/server_middleware.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'opentracing' + +module Gitlab + module Tracing + module Sidekiq + class ServerMiddleware + include SidekiqCommon + + SPAN_KIND = 'server' + + def call(worker, job, queue) + context = tracer.extract(OpenTracing::FORMAT_TEXT_MAP, job) + + in_tracing_span( + operation_name: "sidekiq:#{job['class']}", + child_of: context, + tags: tags_from_job(job, SPAN_KIND)) do |span| + yield + end + end + end + end + end +end diff --git a/lib/gitlab/tracing/sidekiq/sidekiq_common.rb b/lib/gitlab/tracing/sidekiq/sidekiq_common.rb new file mode 100644 index 0000000000000000000000000000000000000000..a911a29d7733b9d7f161e5f559d6941d974feb77 --- /dev/null +++ b/lib/gitlab/tracing/sidekiq/sidekiq_common.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Gitlab + module Tracing + module Sidekiq + module SidekiqCommon + include Gitlab::Tracing::Common + + def tags_from_job(job, kind) + { + 'component' => 'sidekiq', + 'span.kind' => kind, + 'sidekiq.queue' => job['queue'], + 'sidekiq.jid' => job['jid'], + 'sidekiq.retry' => job['retry'].to_s, + 'sidekiq.args' => job['args']&.join(", ") + } + end + end + end + end +end diff --git a/qa/qa/page/label/index.rb b/qa/qa/page/label/index.rb index 9344371a0b6e26955c4ab950a874b41001c41d2b..97ce8f0eba5480606edc634d6bf287c3b76c70f7 100644 --- a/qa/qa/page/label/index.rb +++ b/qa/qa/page/label/index.rb @@ -2,7 +2,7 @@ module QA module Page module Label class Index < Page::Base - view 'app/views/projects/labels/index.html.haml' do + view 'app/views/shared/labels/_nav.html.haml' do element :label_create_new end diff --git a/spec/features/projects/labels/update_prioritization_spec.rb b/spec/features/projects/labels/update_prioritization_spec.rb index 055a0c83a1107f3fe9c2bc2d8fe3bd736ce43bf6..d36f043f8800eac8e0ecbb0ec468234c8bc5a5dd 100644 --- a/spec/features/projects/labels/update_prioritization_spec.rb +++ b/spec/features/projects/labels/update_prioritization_spec.rb @@ -125,7 +125,7 @@ describe 'Prioritize labels' do wait_for_requests end - page.within('.breadcrumbs-container') do + page.within('.top-area') do expect(page).to have_link('New label') end end diff --git a/spec/lib/gitlab/tracing/grpc_interceptor_spec.rb b/spec/lib/gitlab/tracing/grpc_interceptor_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7f5aecb7baa965b42a9a7aa9ffa08e8b8a4af503 --- /dev/null +++ b/spec/lib/gitlab/tracing/grpc_interceptor_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::Tracing::GRPCInterceptor do + subject { described_class.instance } + + shared_examples_for "a grpc interceptor method" do + let(:custom_error) { Class.new(StandardError) } + + it 'yields' do + expect { |b| method.call(kwargs, &b) }.to yield_control + end + + it 'propagates exceptions' do + expect { method.call(kwargs) { raise custom_error } }.to raise_error(custom_error) + end + end + + describe '#request_response' do + let(:method) { subject.method(:request_response) } + let(:kwargs) { { request: {}, call: {}, method: 'grc_method', metadata: {} } } + + it_behaves_like 'a grpc interceptor method' + end + + describe '#client_streamer' do + let(:method) { subject.method(:client_streamer) } + let(:kwargs) { { requests: [], call: {}, method: 'grc_method', metadata: {} } } + + it_behaves_like 'a grpc interceptor method' + end + + describe '#server_streamer' do + let(:method) { subject.method(:server_streamer) } + let(:kwargs) { { request: {}, call: {}, method: 'grc_method', metadata: {} } } + + it_behaves_like 'a grpc interceptor method' + end + + describe '#bidi_streamer' do + let(:method) { subject.method(:bidi_streamer) } + let(:kwargs) { { requests: [], call: {}, method: 'grc_method', metadata: {} } } + + it_behaves_like 'a grpc interceptor method' + end +end diff --git a/spec/lib/gitlab/tracing/rack_middleware_spec.rb b/spec/lib/gitlab/tracing/rack_middleware_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..13d4d8a89f7e4ec0f5f224522c9f98aff0cb5fca --- /dev/null +++ b/spec/lib/gitlab/tracing/rack_middleware_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Tracing::RackMiddleware do + using RSpec::Parameterized::TableSyntax + + describe '#call' do + context 'for normal middleware flow' do + let(:fake_app) { -> (env) { fake_app_response } } + subject { described_class.new(fake_app) } + let(:request) { } + + context 'for 200 responses' do + let(:fake_app_response) { [200, { 'Content-Type': 'text/plain' }, ['OK']] } + + it 'delegates correctly' do + expect(subject.call(Rack::MockRequest.env_for("/"))).to eq(fake_app_response) + end + end + + context 'for 500 responses' do + let(:fake_app_response) { [500, { 'Content-Type': 'text/plain' }, ['Error']] } + + it 'delegates correctly' do + expect(subject.call(Rack::MockRequest.env_for("/"))).to eq(fake_app_response) + end + end + end + + context 'when an application is raising an exception' do + let(:custom_error) { Class.new(StandardError) } + let(:fake_app) { ->(env) { raise custom_error } } + + subject { described_class.new(fake_app) } + + it 'delegates propagates exceptions correctly' do + expect { subject.call(Rack::MockRequest.env_for("/")) }.to raise_error(custom_error) + end + end + end + + describe '.build_sanitized_url_from_env' do + def env_for_url(url) + env = Rack::MockRequest.env_for(input_url) + env['action_dispatch.parameter_filter'] = [/token/] + + env + end + + where(:input_url, :output_url) do + '/gitlab-org/gitlab-ce' | 'http://example.org/gitlab-org/gitlab-ce' + '/gitlab-org/gitlab-ce?safe=1' | 'http://example.org/gitlab-org/gitlab-ce?safe=1' + '/gitlab-org/gitlab-ce?private_token=secret' | 'http://example.org/gitlab-org/gitlab-ce?private_token=%5BFILTERED%5D' + '/gitlab-org/gitlab-ce?mixed=1&private_token=secret' | 'http://example.org/gitlab-org/gitlab-ce?mixed=1&private_token=%5BFILTERED%5D' + end + + with_them do + it { expect(described_class.build_sanitized_url_from_env(env_for_url(input_url))).to eq(output_url) } + end + end +end diff --git a/spec/lib/gitlab/tracing/sidekiq/client_middleware_spec.rb b/spec/lib/gitlab/tracing/sidekiq/client_middleware_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3755860b5bad480b3058fc7e96904e72033195dc --- /dev/null +++ b/spec/lib/gitlab/tracing/sidekiq/client_middleware_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::Tracing::Sidekiq::ClientMiddleware do + describe '#call' do + let(:worker_class) { 'test_worker_class' } + let(:job) do + { + 'class' => "jobclass", + 'queue' => "jobqueue", + 'retry' => 0, + 'args' => %w{1 2 3} + } + end + let(:queue) { 'test_queue' } + let(:redis_pool) { double("redis_pool") } + let(:custom_error) { Class.new(StandardError) } + let(:span) { OpenTracing.start_span('test', ignore_active_scope: true) } + + subject { described_class.new } + + it 'yields' do + expect(subject).to receive(:in_tracing_span).with( + operation_name: "sidekiq:jobclass", + tags: { + "component" => "sidekiq", + "span.kind" => "client", + "sidekiq.queue" => "jobqueue", + "sidekiq.jid" => nil, + "sidekiq.retry" => "0", + "sidekiq.args" => "1, 2, 3" + } + ).and_yield(span) + + expect { |b| subject.call(worker_class, job, queue, redis_pool, &b) }.to yield_control + end + + it 'propagates exceptions' do + expect { subject.call(worker_class, job, queue, redis_pool) { raise custom_error } }.to raise_error(custom_error) + end + end +end diff --git a/spec/lib/gitlab/tracing/sidekiq/server_middleware_spec.rb b/spec/lib/gitlab/tracing/sidekiq/server_middleware_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c3087de785ad8924ea5e11bfdda49fa74895faef --- /dev/null +++ b/spec/lib/gitlab/tracing/sidekiq/server_middleware_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::Tracing::Sidekiq::ServerMiddleware do + describe '#call' do + let(:worker_class) { 'test_worker_class' } + let(:job) do + { + 'class' => "jobclass", + 'queue' => "jobqueue", + 'retry' => 0, + 'args' => %w{1 2 3} + } + end + let(:queue) { 'test_queue' } + let(:custom_error) { Class.new(StandardError) } + let(:span) { OpenTracing.start_span('test', ignore_active_scope: true) } + subject { described_class.new } + + it 'yields' do + expect(subject).to receive(:in_tracing_span).with( + hash_including( + operation_name: "sidekiq:jobclass", + tags: { + "component" => "sidekiq", + "span.kind" => "server", + "sidekiq.queue" => "jobqueue", + "sidekiq.jid" => nil, + "sidekiq.retry" => "0", + "sidekiq.args" => "1, 2, 3" + } + ) + ).and_yield(span) + + expect { |b| subject.call(worker_class, job, queue, &b) }.to yield_control + end + + it 'propagates exceptions' do + expect { subject.call(worker_class, job, queue) { raise custom_error } }.to raise_error(custom_error) + end + end +end