diff options
92 files changed, 954 insertions, 417 deletions
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index ddd698fefeb..e4001e94478 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -490,6 +490,8 @@ export const historyPushState = newUrl => { */ export const parseBoolean = value => (value && value.toString()) === 'true'; +export const BACKOFF_TIMEOUT = 'BACKOFF_TIMEOUT'; + /** * @callback backOffCallback * @param {Function} next @@ -541,7 +543,7 @@ export const backOff = (fn, timeout = 60000) => { timeElapsed += nextInterval; nextInterval = Math.min(nextInterval + nextInterval, maxInterval); } else { - reject(new Error('BACKOFF_TIMEOUT')); + reject(new Error(BACKOFF_TIMEOUT)); } }; diff --git a/app/assets/javascripts/lib/utils/http_status.js b/app/assets/javascripts/lib/utils/http_status.js index 5e5d10883a3..1c7d59054dc 100644 --- a/app/assets/javascripts/lib/utils/http_status.js +++ b/app/assets/javascripts/lib/utils/http_status.js @@ -21,6 +21,7 @@ const httpStatusCodes = { NOT_FOUND: 404, GONE: 410, UNPROCESSABLE_ENTITY: 422, + SERVICE_UNAVAILABLE: 503, }; export const successCodes = [ diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index 6a61d92d9e8..cf1caed5cee 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -245,3 +245,38 @@ export function objectToQuery(obj) { .map(k => `${encodeURIComponent(k)}=${encodeURIComponent(obj[k])}`) .join('&'); } + +/** + * Sets query params for a given URL + * It adds new query params, updates existing params with a new value and removes params with value null/undefined + * + * @param {Object} params The query params to be set/updated + * @param {String} url The url to be operated on + * @param {Boolean} clearParams Indicates whether existing query params should be removed or not + * @returns {String} A copy of the original with the updated query params + */ +export const setUrlParams = (params, url = window.location.href, clearParams = false) => { + const urlObj = new URL(url); + const queryString = urlObj.search; + const searchParams = clearParams ? new URLSearchParams('') : new URLSearchParams(queryString); + + Object.keys(params).forEach(key => { + if (params[key] === null || params[key] === undefined) { + searchParams.delete(key); + } else if (Array.isArray(params[key])) { + params[key].forEach((val, idx) => { + if (idx === 0) { + searchParams.set(key, val); + } else { + searchParams.append(key, val); + } + }); + } else { + searchParams.set(key, params[key]); + } + }); + + urlObj.search = searchParams.toString(); + + return urlObj.toString(); +}; diff --git a/app/assets/javascripts/monitoring/constants.js b/app/assets/javascripts/monitoring/constants.js index 1a1fcdd0e66..e613351e524 100644 --- a/app/assets/javascripts/monitoring/constants.js +++ b/app/assets/javascripts/monitoring/constants.js @@ -1,5 +1,42 @@ import { __ } from '~/locale'; +export const PROMETHEUS_TIMEOUT = 120000; // TWO_MINUTES + +/** + * Errors in Prometheus Queries (PromQL) for metrics + */ +export const metricsErrors = { + /** + * Connection timed out to prometheus server + * the timeout is set to PROMETHEUS_TIMEOUT + * + */ + TIMEOUT: 'TIMEOUT', + + /** + * The prometheus server replies with an empty data set + */ + NO_DATA: 'NO_DATA', + + /** + * The prometheus server cannot be reached + */ + CONNECTION_FAILED: 'CONNECTION_FAILED', + + /** + * The prometheus server was reach but it cannot process + * the query. This can happen for several reasons: + * - PromQL syntax is incorrect + * - An operator is not supported + */ + BAD_DATA: 'BAD_DATA', + + /** + * No specific reason found for error + */ + UNKNOWN_ERROR: 'UNKNOWN_ERROR', +}; + export const sidebarAnimationDuration = 300; // milliseconds. export const chartHeight = 300; diff --git a/app/assets/javascripts/monitoring/stores/actions.js b/app/assets/javascripts/monitoring/stores/actions.js index 268d9d636b1..a655191b2b4 100644 --- a/app/assets/javascripts/monitoring/stores/actions.js +++ b/app/assets/javascripts/monitoring/stores/actions.js @@ -6,7 +6,7 @@ import statusCodes from '../../lib/utils/http_status'; import { backOff } from '../../lib/utils/common_utils'; import { s__, sprintf } from '../../locale'; -const TWO_MINUTES = 120000; +import { PROMETHEUS_TIMEOUT } from '../constants'; function backOffRequest(makeRequestCallback) { return backOff((next, stop) => { @@ -19,7 +19,7 @@ function backOffRequest(makeRequestCallback) { } }) .catch(stop); - }, TWO_MINUTES); + }, PROMETHEUS_TIMEOUT); } export const setGettingStartedEmptyState = ({ commit }) => { @@ -125,9 +125,17 @@ export const fetchPrometheusMetric = ({ commit }, { metric, params }) => { step, }; - return fetchPrometheusResult(metric.prometheus_endpoint_path, queryParams).then(result => { - commit(types.SET_QUERY_RESULT, { metricId: metric.metric_id, result }); - }); + commit(types.REQUEST_METRIC_RESULT, { metricId: metric.metric_id }); + + return fetchPrometheusResult(metric.prometheus_endpoint_path, queryParams) + .then(result => { + commit(types.RECEIVE_METRIC_RESULT_SUCCESS, { metricId: metric.metric_id, result }); + }) + .catch(error => { + commit(types.RECEIVE_METRIC_RESULT_ERROR, { metricId: metric.metric_id, error }); + // Continue to throw error so the dashboard can notify using createFlash + throw error; + }); }; export const fetchPrometheusMetrics = ({ state, commit, dispatch, getters }, params) => { @@ -159,7 +167,8 @@ export const fetchDeploymentsData = ({ state, dispatch }) => { if (!state.deploymentsEndpoint) { return Promise.resolve([]); } - return backOffRequest(() => axios.get(state.deploymentsEndpoint)) + return axios + .get(state.deploymentsEndpoint) .then(resp => resp.data) .then(response => { if (!response || !response.deployments) { diff --git a/app/assets/javascripts/monitoring/stores/mutation_types.js b/app/assets/javascripts/monitoring/stores/mutation_types.js index fa15a2ba800..e4e467f3d68 100644 --- a/app/assets/javascripts/monitoring/stores/mutation_types.js +++ b/app/assets/javascripts/monitoring/stores/mutation_types.js @@ -1,13 +1,19 @@ export const REQUEST_METRICS_DATA = 'REQUEST_METRICS_DATA'; export const RECEIVE_METRICS_DATA_SUCCESS = 'RECEIVE_METRICS_DATA_SUCCESS'; export const RECEIVE_METRICS_DATA_FAILURE = 'RECEIVE_METRICS_DATA_FAILURE'; + export const REQUEST_DEPLOYMENTS_DATA = 'REQUEST_DEPLOYMENTS_DATA'; export const RECEIVE_DEPLOYMENTS_DATA_SUCCESS = 'RECEIVE_DEPLOYMENTS_DATA_SUCCESS'; export const RECEIVE_DEPLOYMENTS_DATA_FAILURE = 'RECEIVE_DEPLOYMENTS_DATA_FAILURE'; + export const REQUEST_ENVIRONMENTS_DATA = 'REQUEST_ENVIRONMENTS_DATA'; export const RECEIVE_ENVIRONMENTS_DATA_SUCCESS = 'RECEIVE_ENVIRONMENTS_DATA_SUCCESS'; export const RECEIVE_ENVIRONMENTS_DATA_FAILURE = 'RECEIVE_ENVIRONMENTS_DATA_FAILURE'; -export const SET_QUERY_RESULT = 'SET_QUERY_RESULT'; + +export const REQUEST_METRIC_RESULT = 'REQUEST_METRIC_RESULT'; +export const RECEIVE_METRIC_RESULT_SUCCESS = 'RECEIVE_METRIC_RESULT_SUCCESS'; +export const RECEIVE_METRIC_RESULT_ERROR = 'RECEIVE_METRIC_RESULT_ERROR'; + export const SET_TIME_WINDOW = 'SET_TIME_WINDOW'; export const SET_ALL_DASHBOARDS = 'SET_ALL_DASHBOARDS'; export const SET_ENDPOINTS = 'SET_ENDPOINTS'; diff --git a/app/assets/javascripts/monitoring/stores/mutations.js b/app/assets/javascripts/monitoring/stores/mutations.js index db5ec4e9e2b..f04c12c2ac8 100644 --- a/app/assets/javascripts/monitoring/stores/mutations.js +++ b/app/assets/javascripts/monitoring/stores/mutations.js @@ -2,6 +2,9 @@ import Vue from 'vue'; import { slugify } from '~/lib/utils/text_utility'; import * as types from './mutation_types'; import { normalizeMetric, normalizeQueryResult } from './utils'; +import { BACKOFF_TIMEOUT } from '../../lib/utils/common_utils'; +import { metricsErrors } from '../constants'; +import httpStatusCodes from '~/lib/utils/http_status'; const normalizePanelMetrics = (metrics, defaultLabel) => metrics.map(metric => ({ @@ -9,7 +12,74 @@ const normalizePanelMetrics = (metrics, defaultLabel) => label: metric.label || defaultLabel, })); +/** + * Locate and return a metric in the dashboard by its id + * as generated by `uniqMetricsId()`. + * @param {String} metricId Unique id in the dashboard + * @param {Object} dashboard Full dashboard object + */ +const findMetricInDashboard = (metricId, dashboard) => { + let res = null; + dashboard.panel_groups.forEach(group => { + group.panels.forEach(panel => { + panel.metrics.forEach(metric => { + if (metric.metric_id === metricId) { + res = metric; + } + }); + }); + }); + return res; +}; + +/** + * Set a new state for a metric. + * + * Initally metric data is not populated, so `Vue.set` is + * used to add new properties to the metric. + * + * @param {Object} metric - Metric object as defined in the dashboard + * @param {Object} state - New state + * @param {Array|null} state.result - Array of results + * @param {String} state.error - Error code from metricsErrors + * @param {Boolean} state.loading - True if the metric is loading + */ +const setMetricState = (metric, { result = null, error = null, loading = false }) => { + Vue.set(metric, 'result', result); + Vue.set(metric, 'error', error); + Vue.set(metric, 'loading', loading); +}; + +/** + * Maps a backened error state to a `metricsErrors` constant + * @param {Object} error - Error from backend response + */ +const getMetricError = error => { + if (!error) { + return metricsErrors.UNKNOWN_ERROR; + } + + // Special error responses + if (error.message === BACKOFF_TIMEOUT) { + return metricsErrors.TIMEOUT; + } + + // Axios error responses + const { response } = error; + if (response && response.status === httpStatusCodes.SERVICE_UNAVAILABLE) { + return metricsErrors.CONNECTION_FAILED; + } else if (response && response.status === httpStatusCodes.BAD_REQUEST) { + // Note: "error.response.data.error" may contain Prometheus error information + return metricsErrors.BAD_DATA; + } + + return metricsErrors.UNKNOWN_ERROR; +}; + export default { + /** + * Dashboard panels structure and global state + */ [types.REQUEST_METRICS_DATA](state) { state.emptyState = 'loading'; state.showEmptyState = true; @@ -40,6 +110,10 @@ export default { state.emptyState = error ? 'unableToConnect' : 'noData'; state.showEmptyState = true; }, + + /** + * Deployments and environments + */ [types.RECEIVE_DEPLOYMENTS_DATA_SUCCESS](state, deployments) { state.deploymentData = deployments; }, @@ -53,28 +127,46 @@ export default { state.environments = []; }, - [types.SET_QUERY_RESULT](state, { metricId, result }) { - if (!metricId || !result || result.length === 0) { + /** + * Individual panel/metric results + */ + [types.REQUEST_METRIC_RESULT](state, { metricId }) { + const metric = findMetricInDashboard(metricId, state.dashboard); + + setMetricState(metric, { + loading: true, + }); + }, + [types.RECEIVE_METRIC_RESULT_SUCCESS](state, { metricId, result }) { + if (!metricId) { return; } state.showEmptyState = false; - /** - * Search the dashboard state for a matching id - */ - state.dashboard.panel_groups.forEach(group => { - group.panels.forEach(panel => { - panel.metrics.forEach(metric => { - if (metric.metric_id === metricId) { - // ensure dates/numbers are correctly formatted for charts - const normalizedResults = result.map(normalizeQueryResult); - Vue.set(metric, 'result', Object.freeze(normalizedResults)); - } - }); + const metric = findMetricInDashboard(metricId, state.dashboard); + if (!result || result.length === 0) { + // If no data is return we still consider it an error and set it to undefined + setMetricState(metric, { + error: metricsErrors.NO_DATA, + }); + } else { + const normalizedResults = result.map(normalizeQueryResult); + setMetricState(metric, { + result: Object.freeze(normalizedResults), }); + } + }, + [types.RECEIVE_METRIC_RESULT_ERROR](state, { metricId, error }) { + if (!metricId) { + return; + } + const metric = findMetricInDashboard(metricId, state.dashboard); + setMetricState(metric, { + error: getMetricError(error), }); }, + [types.SET_ENDPOINTS](state, endpoints) { state.metricsEndpoint = endpoints.metricsEndpoint; state.environmentsEndpoint = endpoints.environmentsEndpoint; diff --git a/app/assets/javascripts/monitoring/stores/state.js b/app/assets/javascripts/monitoring/stores/state.js index 88f333aeb80..ee8a85ea222 100644 --- a/app/assets/javascripts/monitoring/stores/state.js +++ b/app/assets/javascripts/monitoring/stores/state.js @@ -8,9 +8,11 @@ export default () => ({ emptyState: 'gettingStarted', showEmptyState: true, showErrorBanner: true, + dashboard: { panel_groups: [], }, + deploymentData: [], environments: [], allDashboards: [], diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1ed8da57927..ba986c495e2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -23,7 +23,7 @@ class ApplicationController < ActionController::Base before_action :validate_user_service_ticket! before_action :check_password_expiration, if: :html_request? before_action :ldap_security_check - before_action :sentry_context + around_action :sentry_context before_action :default_headers before_action :add_gon_variables, if: :html_request? before_action :configure_permitted_parameters, if: :devise_controller? @@ -165,7 +165,7 @@ class ApplicationController < ActionController::Base end def log_exception(exception) - Gitlab::Sentry.track_acceptable_exception(exception) + Gitlab::Sentry.track_exception(exception) backtrace_cleaner = request.env["action_dispatch.backtrace_cleaner"] application_trace = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, exception).application_trace @@ -532,8 +532,8 @@ class ApplicationController < ActionController::Base @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id] end - def sentry_context - Gitlab::Sentry.context(current_user) + def sentry_context(&block) + Gitlab::Sentry.with_context(current_user, &block) end def allow_gitaly_ref_name_caching diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 7b7cfa7a5d3..4a1b06bf01f 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -98,13 +98,11 @@ module IssuableActions error_message = "Destroy confirmation not provided for #{issuable.human_class_name}" exception = RuntimeError.new(error_message) - Gitlab::Sentry.track_acceptable_exception( + Gitlab::Sentry.track_exception( exception, - extra: { - project_path: issuable.project.full_path, - issuable_type: issuable.class.name, - issuable_id: issuable.id - } + project_path: issuable.project.full_path, + issuable_type: issuable.class.name, + issuable_id: issuable.id ) index_path = polymorphic_path([parent, issuable.class]) diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb index 4f73270577f..b79766e3a65 100644 --- a/app/helpers/icons_helper.rb +++ b/app/helpers/icons_helper.rb @@ -42,11 +42,9 @@ module IconsHelper end def sprite_icon(icon_name, size: nil, css_class: nil) - if Gitlab::Sentry.should_raise_for_dev? - unless known_sprites.include?(icon_name) - exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg") - raise exception - end + unless known_sprites.include?(icon_name) + exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg") + Gitlab::Sentry.track_and_raise_for_dev_exception(exception) end css_classes = [] diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index ef0cb8b4bcb..ee3c03905ef 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -57,7 +57,7 @@ module UsersHelper unless user.association(:status).loaded? exception = RuntimeError.new("Status was not preloaded") - Gitlab::Sentry.track_exception(exception, extra: { user: user.inspect }) + Gitlab::Sentry.track_and_raise_for_dev_exception(exception, user: user.inspect) end return unless user.status diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 21dbbae1747..2e6b5d68747 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -289,7 +289,7 @@ module Ci begin build.deployment.drop! rescue => e - Gitlab::Sentry.track_exception(e, extra: { build_id: build.id }) + Gitlab::Sentry.track_and_raise_for_dev_exception(e, build_id: build.id) end true diff --git a/app/models/ci/persistent_ref.rb b/app/models/ci/persistent_ref.rb index 10d7795be2b..634c03e0326 100644 --- a/app/models/ci/persistent_ref.rb +++ b/app/models/ci/persistent_ref.rb @@ -27,7 +27,7 @@ module Ci create_ref(sha, path) rescue => e Gitlab::Sentry - .track_acceptable_exception(e, extra: { pipeline_id: pipeline.id }) + .track_exception(e, pipeline_id: pipeline.id) end def delete @@ -38,7 +38,7 @@ module Ci # no-op rescue => e Gitlab::Sentry - .track_acceptable_exception(e, extra: { pipeline_id: pipeline.id }) + .track_exception(e, pipeline_id: pipeline.id) end def path diff --git a/app/models/clusters/applications/elastic_stack.rb b/app/models/clusters/applications/elastic_stack.rb index 8589f8c00cb..9854ad2ea3e 100644 --- a/app/models/clusters/applications/elastic_stack.rb +++ b/app/models/clusters/applications/elastic_stack.rb @@ -71,6 +71,8 @@ module Clusters # `proxy_url` could raise an exception because gitlab can not communicate with the cluster. # We check for a nil client in downstream use and behaviour is equivalent to an empty state log_exception(error, :failed_to_create_elasticsearch_client) + + nil end end diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 3742bbd8446..2d5b4905bf5 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -335,7 +335,7 @@ module Clusters rescue Kubeclient::HttpError => e kubeclient_error_status(e.message) rescue => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { cluster_id: id }) + Gitlab::Sentry.track_exception(e, cluster_id: id) :unknown_failure else diff --git a/app/models/clusters/concerns/application_core.rb b/app/models/clusters/concerns/application_core.rb index 21b98534808..cc9eafbcdd6 100644 --- a/app/models/clusters/concerns/application_core.rb +++ b/app/models/clusters/concerns/application_core.rb @@ -76,7 +76,7 @@ module Clusters message: error.message }) - Gitlab::Sentry.track_acceptable_exception(error, extra: { cluster_id: cluster&.id, application_id: id }) + Gitlab::Sentry.track_exception(error, cluster_id: cluster&.id, application_id: id) end end end diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 7e6a20c27e8..18f6a7a9c58 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -48,11 +48,11 @@ module GroupDescendant extras = { parent: parent.inspect, child: child.inspect, - preloaded: preloaded.map(&:full_path) + preloaded: preloaded.map(&:full_path), + issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/49404' } - issue_url = 'https://gitlab.com/gitlab-org/gitlab-foss/issues/49404' - Gitlab::Sentry.track_exception(exception, issue_url: issue_url, extra: extras) + Gitlab::Sentry.track_and_raise_for_dev_exception(exception, extras) end if parent.nil? && hierarchy_top.present? diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 9c2b0372d54..b9081f9bbf2 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -37,8 +37,10 @@ module Storage send_update_instructions write_projects_repository_config rescue => e - # Raise if development/test environment, else just notify Sentry - Gitlab::Sentry.track_exception(e, extra: { full_path_before_last_save: full_path_before_last_save, full_path: full_path, action: 'move_dir' }) + Gitlab::Sentry.track_and_raise_for_dev_exception(e, + full_path_before_last_save: full_path_before_last_save, + full_path: full_path, + action: 'move_dir') end true # false would cancel later callbacks but not rollback diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index bbc54987407..1671b1e2d55 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1514,7 +1514,7 @@ class MergeRequest < ApplicationRecord end end rescue ActiveRecord::LockWaitTimeout => e - Gitlab::Sentry.track_acceptable_exception(e) + Gitlab::Sentry.track_exception(e) raise RebaseLockTimeout, REBASE_LOCK_MESSAGE end diff --git a/app/models/upload.rb b/app/models/upload.rb index 650321e2793..12917f85431 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -103,10 +103,8 @@ class Upload < ApplicationRecord # Help sysadmins find missing upload files if persisted? && !exist - if Gitlab::Sentry.enabled? - Raven.capture_message(_("Upload file does not exist"), extra: self.attributes) - end - + exception = RuntimeError.new("Uploaded file does not exist") + Gitlab::Sentry.track_exception(exception, self.attributes) Gitlab::Metrics.counter(:upload_file_does_not_exist_total, _('The number of times an upload record could not find its file')).increment end diff --git a/app/models/uploads/local.rb b/app/models/uploads/local.rb index 2901c33c359..f1f25dfb584 100644 --- a/app/models/uploads/local.rb +++ b/app/models/uploads/local.rb @@ -23,7 +23,8 @@ module Uploads unless in_uploads?(path) message = "Path '#{path}' is not in uploads dir, skipping" logger.warn(message) - Gitlab::Sentry.track_exception(RuntimeError.new(message), extra: { uploads_dir: storage_dir }) + Gitlab::Sentry.track_and_raise_for_dev_exception( + RuntimeError.new(message), uploads_dir: storage_dir) return end diff --git a/app/services/ci/archive_trace_service.rb b/app/services/ci/archive_trace_service.rb index 8fad9e9c869..75c7eee2f72 100644 --- a/app/services/ci/archive_trace_service.rb +++ b/app/services/ci/archive_trace_service.rb @@ -47,9 +47,9 @@ module Ci job_id: job.id) Gitlab::Sentry - .track_exception(error, + .track_and_raise_for_dev_exception(error, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/51502', - extra: { job_id: job.id }) + job_id: job.id ) end end end diff --git a/app/services/ci/generate_exposed_artifacts_report_service.rb b/app/services/ci/generate_exposed_artifacts_report_service.rb index b9bf580bcbc..af6331341ff 100644 --- a/app/services/ci/generate_exposed_artifacts_report_service.rb +++ b/app/services/ci/generate_exposed_artifacts_report_service.rb @@ -15,7 +15,7 @@ module Ci data: data } rescue => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { project_id: project.id }) + Gitlab::Sentry.track_exception(e, project_id: project.id) { status: :error, key: key(base_pipeline, head_pipeline), diff --git a/app/services/ci/prepare_build_service.rb b/app/services/ci/prepare_build_service.rb index 3722faeb020..8ace7914f8e 100644 --- a/app/services/ci/prepare_build_service.rb +++ b/app/services/ci/prepare_build_service.rb @@ -13,7 +13,7 @@ module Ci build.enqueue! rescue => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { build_id: build.id }) + Gitlab::Sentry.track_exception(e, build_id: build.id) build.drop(:unmet_prerequisites) end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 30e2a66e04a..24597579d9e 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -128,13 +128,13 @@ module Ci end def track_exception_for_build(ex, build) - Gitlab::Sentry.track_acceptable_exception(ex, extra: { + Gitlab::Sentry.track_exception(ex, build_id: build.id, build_name: build.name, build_stage: build.stage, pipeline_id: build.pipeline_id, project_id: build.project_id - }) + ) end # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/services/clusters/applications/base_helm_service.rb b/app/services/clusters/applications/base_helm_service.rb index 3e7f55f0c63..2b51de71934 100644 --- a/app/services/clusters/applications/base_helm_service.rb +++ b/app/services/clusters/applications/base_helm_service.rb @@ -21,14 +21,7 @@ module Clusters group_ids: app.cluster.group_ids } - logger_meta = meta.merge( - exception: error.class.name, - message: error.message, - backtrace: Gitlab::Profiler.clean_backtrace(error.backtrace) - ) - - logger.error(logger_meta) - Gitlab::Sentry.track_acceptable_exception(error, extra: meta) + Gitlab::Sentry.track_exception(error, meta) end def log_event(event) diff --git a/app/services/projects/container_repository/delete_tags_service.rb b/app/services/projects/container_repository/delete_tags_service.rb index af58d3780b0..0da3ddea9f7 100644 --- a/app/services/projects/container_repository/delete_tags_service.rb +++ b/app/services/projects/container_repository/delete_tags_service.rb @@ -51,7 +51,7 @@ module Projects digests = deleted_tags.values.uniq # rubocop: disable CodeReuse/ActiveRecord - Gitlab::Sentry.track_exception(ArgumentError.new('multiple tag digests')) if digests.many? + Gitlab::Sentry.track_and_raise_for_dev_exception(ArgumentError.new('multiple tag digests')) if digests.many? deleted_tags end diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 073c14040ce..bef4897baec 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -25,13 +25,13 @@ module Projects success rescue Gitlab::UrlBlocker::BlockedUrlError => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + Gitlab::Sentry.track_exception(e, project_path: project.full_path, importer: project.import_type) error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: e.message }) rescue => e message = Projects::ImportErrorFilter.filter_message(e.message) - Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + Gitlab::Sentry.track_exception(e, project_path: project.full_path, importer: project.import_type) error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: message }) end diff --git a/app/services/prometheus/proxy_variable_substitution_service.rb b/app/services/prometheus/proxy_variable_substitution_service.rb index d3d56987f07..4a6678ec237 100644 --- a/app/services/prometheus/proxy_variable_substitution_service.rb +++ b/app/services/prometheus/proxy_variable_substitution_service.rb @@ -32,7 +32,7 @@ module Prometheus success(result) rescue TypeError, ArgumentError => exception log_error(exception.message) - Gitlab::Sentry.track_acceptable_exception(exception, extra: { + Gitlab::Sentry.track_exception(exception, extra: { template_string: query, variables: predefined_context }) diff --git a/app/workers/delete_stored_files_worker.rb b/app/workers/delete_stored_files_worker.rb index 8a693a64055..1d52f71c866 100644 --- a/app/workers/delete_stored_files_worker.rb +++ b/app/workers/delete_stored_files_worker.rb @@ -15,7 +15,7 @@ class DeleteStoredFilesWorker unless klass message = "Unknown class '#{class_name}'" logger.error(message) - Gitlab::Sentry.track_exception(RuntimeError.new(message)) + Gitlab::Sentry.track_and_raise_for_dev_exception(RuntimeError.new(message)) return end diff --git a/app/workers/pages_domain_removal_cron_worker.rb b/app/workers/pages_domain_removal_cron_worker.rb index b1506831056..ba3c89d0e70 100644 --- a/app/workers/pages_domain_removal_cron_worker.rb +++ b/app/workers/pages_domain_removal_cron_worker.rb @@ -11,7 +11,7 @@ class PagesDomainRemovalCronWorker PagesDomain.for_removal.find_each do |domain| domain.destroy! rescue => e - Raven.capture_exception(e) + Gitlab::Sentry.track_exception(e) end end end diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb index 853f774875a..450dee0e83e 100644 --- a/app/workers/run_pipeline_schedule_worker.rb +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -39,9 +39,9 @@ class RunPipelineScheduleWorker "schedule_id: #{schedule.id} message: #{error.message}" Gitlab::Sentry - .track_exception(error, + .track_and_raise_for_dev_exception(error, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/41231', - extra: { schedule_id: schedule.id }) + schedule_id: schedule.id) end # rubocop:enable Gitlab/RailsLogger diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index b116965d105..99eff044eae 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -80,12 +80,12 @@ class StuckCiJobsWorker end def track_exception_for_build(ex, build) - Gitlab::Sentry.track_acceptable_exception(ex, extra: { + Gitlab::Sentry.track_exception(ex, build_id: build.id, build_name: build.name, build_stage: build.stage, pipeline_id: build.pipeline_id, project_id: build.project_id - }) + ) end end diff --git a/changelogs/unreleased/13075-document-gradle-support.yml b/changelogs/unreleased/13075-document-gradle-support.yml new file mode 100644 index 00000000000..78c0489d38c --- /dev/null +++ b/changelogs/unreleased/13075-document-gradle-support.yml @@ -0,0 +1,5 @@ +--- +title: Add documentation about dependency scanning gradle support +merge_request: 21253 +author: +type: added diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb index bb190af60b5..bd59fd4ab90 100644 --- a/config/initializers/forbid_sidekiq_in_transactions.rb +++ b/config/initializers/forbid_sidekiq_in_transactions.rb @@ -29,7 +29,7 @@ module Sidekiq MSG rescue Sidekiq::Worker::EnqueueFromTransactionError => e ::Rails.logger.error(e.message) if ::Rails.env.production? - Gitlab::Sentry.track_exception(e) + Gitlab::Sentry.track_and_raise_for_dev_exception(e) end end diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb index 48daca3d254..cebb0edf275 100644 --- a/config/initializers/sentry.rb +++ b/config/initializers/sentry.rb @@ -2,35 +2,4 @@ require 'gitlab/current_settings' -def configure_sentry - if Gitlab::Sentry.enabled? - Raven.configure do |config| - config.dsn = Gitlab.config.sentry.dsn - config.release = Gitlab.revision - config.current_environment = Gitlab.config.sentry.environment - - # Sanitize fields based on those sanitized from Rails. - config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s) - # Sanitize authentication headers - config.sanitize_http_headers = %w[Authorization Private-Token] - config.tags = { program: Gitlab.process_name } - # Debugging for https://gitlab.com/gitlab-org/gitlab-foss/issues/57727 - config.before_send = lambda do |event, hint| - if ActiveModel::MissingAttributeError === hint[:exception] - columns_hash = ActiveRecord::Base - .connection - .schema_cache - .instance_variable_get(:@columns_hash) - .map { |k, v| [k, v.map(&:first)] } - .to_h - - event.extra.merge!(columns_hash) - end - - event - end - end - end -end - -configure_sentry if Rails.env.production? || Rails.env.development? +Gitlab::Sentry.configure diff --git a/doc/administration/monitoring/prometheus/postgres_exporter.md b/doc/administration/monitoring/prometheus/postgres_exporter.md index 044ce64af53..27771865e10 100644 --- a/doc/administration/monitoring/prometheus/postgres_exporter.md +++ b/doc/administration/monitoring/prometheus/postgres_exporter.md @@ -1,12 +1,12 @@ -# Postgres exporter +# PostgreSQL Server Exporter >**Note:** -Available since [Omnibus GitLab 8.17][1131]. For installations from source -you'll have to install and configure it yourself. +Available since [Omnibus GitLab 8.17](https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1131). +For installations from source you will have to install and configure it yourself. -The [postgres exporter] allows you to measure various PostgreSQL metrics. +The [PostgreSQL Server Exporter](https://github.com/wrouesnel/postgres_exporter) allows you to export various PostgreSQL metrics. -To enable the postgres exporter: +To enable the PostgreSQL Server Exporter: 1. [Enable Prometheus](index.md#configuring-prometheus). 1. Edit `/etc/gitlab/gitlab.rb` and enable `postgres_exporter`: @@ -16,23 +16,22 @@ To enable the postgres exporter: ``` NOTE: **Note:** -If PostgreSQL is configured on a separate node, make sure that the local +If PostgreSQL Server Exporter is configured on a separate node, make sure that the local address is [listed in `trust_auth_cidr_addresses`](../../high_availability/database.md#network-information) or the exporter will not be able to connect to the database. -1. Save the file and [reconfigure GitLab][reconfigure] for the changes to +1. Save the file and [reconfigure GitLab](../../restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect. Prometheus will now automatically begin collecting performance data from -the postgres exporter exposed under `localhost:9187`. +the PostgreSQL Server Exporter exposed under `localhost:9187`. ## Advanced configuration -In most cases, Postgres exporter will work with the defaults and you should not +In most cases, PostgreSQL Server Exporter will work with the defaults and you should not need to change anything. -The following configuration options can be used to further customize the -Postgres exporter: +To further customize the PostgreSQL Server Exporter, use the following configuration options: 1. Edit `/etc/gitlab/gitlab.rb`: @@ -54,11 +53,6 @@ Postgres exporter: postgres_exporter['sslrootcert'] = 'ssl-root.crt' # The location of the root certificate file. The file must contain PEM encoded data. ``` -1. Save the file and [reconfigure GitLab][reconfigure] for the changes to take effect. +1. Save the file and [reconfigure GitLab](../../restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect. [← Back to the main Prometheus page](index.md) - -[1131]: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1131 -[postgres exporter]: https://github.com/wrouesnel/postgres_exporter -[prometheus]: https://prometheus.io -[reconfigure]: ../../restart_gitlab.md#omnibus-gitlab-reconfigure diff --git a/doc/development/logging.md b/doc/development/logging.md index 22f3059d4b2..4ccb5a1a06e 100644 --- a/doc/development/logging.md +++ b/doc/development/logging.md @@ -127,6 +127,68 @@ importer progresses. Here's what to do: logger.info(message: "Import error", error_code: 1, error: "I/O failure") ``` +## Exception Handling + +It often happens that you catch the exception and want to track it. + +It should be noted that manual logging of exceptions is not allowed, as: + +1. Manual logged exceptions can leak confidential data, +1. Manual logged exception very often require to clean backtrace + which reduces the boilerplate, +1. Very often manually logged exception needs to be tracked to Sentry as well, +1. Manually logged exceptions does not use `correlation_id`, which makes hard + to pin them to request, user and context in which this exception was raised, +1. It is very likely that manually logged exceptions will end-up across + multiple files, which increases burden scraping all logging files. + +To avoid duplicating and having consistent behavior the `Gitlab::Sentry` +provides helper methods to track exceptions: + +1. `Gitlab::Sentry.track_and_raise_exception`: this method logs, + sends exception to Sentry (if configured) and re-raises the exception, +1. `Gitlab::Sentry.track_exception`: this method only logs + and sends exception to Sentry (if configured), +1. `Gitlab::Sentry.log_exception`: this method only logs the exception, + and DOES NOT send the exception to Sentry, +1. `Gitlab::Sentry.track_and_raise_for_dev_exception`: this method logs, + sends exception to Sentry (if configured) and re-raises the exception + for development and test enviroments. + +It is advised to only use `Gitlab::Sentry.track_and_raise_exception` +and `Gitlab::Sentry.track_exception` as presented on below examples. + +Consider adding additional extra parameters to provide more context +for each tracked exception. + +### Example + +```ruby +class MyService < ::BaseService + def execute + project.perform_expensive_operation + + success + rescue => e + Gitlab::Sentry.track_exception(e, project_id: project.id) + + error('Exception occurred') + end +end +``` + +```ruby +class MyService < ::BaseService + def execute + project.perform_expensive_operation + + success + rescue => e + Gitlab::Sentry.track_and_raise_exception(e, project_id: project.id) + end +end +``` + ## Additional steps with new log files 1. Consider log retention settings. By default, Omnibus will rotate any diff --git a/doc/update/upgrading_from_ce_to_ee.md b/doc/update/upgrading_from_ce_to_ee.md index b553b4aa405..52a65a89cbf 100644 --- a/doc/update/upgrading_from_ce_to_ee.md +++ b/doc/update/upgrading_from_ce_to_ee.md @@ -59,17 +59,22 @@ sudo -u git -H git checkout EE_BRANCH ```sh cd /home/git/gitlab -# MySQL installations (note: the line below states '--without postgres') -sudo -u git -H bundle install --without postgres development test --deployment +sudo -u git -H bundle install --deployment --without development test mysql aws kerberos -# PostgreSQL installations (note: the line below states '--without mysql') -sudo -u git -H bundle install --without mysql development test --deployment +# Optional: clean up old gems +sudo -u git -H bundle clean # Run database migrations sudo -u git -H bundle exec rake db:migrate RAILS_ENV=production -# Clean up assets and cache -sudo -u git -H bundle exec rake assets:clean assets:precompile cache:clear RAILS_ENV=production +# Compile GetText PO files +sudo -u git -H bundle exec rake gettext:compile RAILS_ENV=production + +# Update node dependencies and recompile assets +sudo -u git -H bundle exec rake yarn:install gitlab:assets:clean gitlab:assets:compile RAILS_ENV=production NODE_ENV=production NODE_OPTIONS="--max_old_space_size=4096" + +# Clean up cache +sudo -u git -H bundle exec rake cache:clear RAILS_ENV=production ``` ### 4. Install `gitlab-elasticsearch-indexer` (optional) **(STARTER ONLY)** diff --git a/doc/user/application_security/dependency_scanning/index.md b/doc/user/application_security/dependency_scanning/index.md index 61666bc00cd..3f13d5983aa 100644 --- a/doc/user/application_security/dependency_scanning/index.md +++ b/doc/user/application_security/dependency_scanning/index.md @@ -55,7 +55,7 @@ The following languages and dependency managers are supported. | Language (package managers) | Supported | Scan tool(s) | |----------------------------- | --------- | ------------ | -| Java ([Gradle](https://gradle.org/)) | not currently ([issue](https://gitlab.com/gitlab-org/gitlab/issues/13075 "Dependency Scanning for Gradle" )) | not available | +| Java ([Gradle](https://gradle.org/)) | yes | [gemnasium](https://gitlab.com/gitlab-org/security-products/gemnasium) | | Java ([Maven](https://maven.apache.org/)) | yes | [gemnasium](https://gitlab.com/gitlab-org/security-products/gemnasium) | | JavaScript ([npm](https://www.npmjs.com/), [yarn](https://yarnpkg.com/en/)) | yes | [gemnasium](https://gitlab.com/gitlab-org/security-products/gemnasium), [Retire.js](https://retirejs.github.io/retire.js/) | | Go ([Golang](https://golang.org/)) | not currently ([issue](https://gitlab.com/gitlab-org/gitlab/issues/7132 "Dependency Scanning for Go")) | not available | diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 49b86489a8b..a3648ec3df3 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -384,8 +384,9 @@ module API def handle_api_exception(exception) if report_exception?(exception) define_params_for_grape_middleware - Gitlab::Sentry.context(current_user) - Gitlab::Sentry.track_acceptable_exception(exception, extra: params) + Gitlab::Sentry.with_context(current_user) do + Gitlab::Sentry.track_exception(exception, params) + end end # This is used with GrapeLogging::Loggers::ExceptionLogger diff --git a/lib/gitlab/bitbucket_import/importer.rb b/lib/gitlab/bitbucket_import/importer.rb index e01ffb631ba..5fbf7be2568 100644 --- a/lib/gitlab/bitbucket_import/importer.rb +++ b/lib/gitlab/bitbucket_import/importer.rb @@ -11,7 +11,6 @@ module Gitlab { title: 'task', color: '#7F8C8D' }].freeze attr_reader :project, :client, :errors, :users - attr_accessor :logger def initialize(project) @project = project @@ -20,7 +19,6 @@ module Gitlab @labels = {} @errors = [] @users = {} - @logger = Gitlab::Import::Logger.build end def execute @@ -47,7 +45,8 @@ module Gitlab backtrace = Gitlab::Profiler.clean_backtrace(ex.backtrace) error = { type: :pull_request, iid: pull_request.iid, errors: ex.message, trace: backtrace, raw_response: pull_request.raw } - log_error(error) + Gitlab::Sentry.log_exception(ex, error) + # Omit the details from the database to avoid blowing up usage in the error column error.delete(:trace) error.delete(:raw_response) @@ -275,10 +274,6 @@ module Gitlab author.to_s + comment.note.to_s end - def log_error(details) - logger.error(log_base_data.merge(details)) - end - def log_base_data { class: self.class.name, diff --git a/lib/gitlab/bitbucket_server_import/importer.rb b/lib/gitlab/bitbucket_server_import/importer.rb index af50a27c47b..1a9ac65e641 100644 --- a/lib/gitlab/bitbucket_server_import/importer.rb +++ b/lib/gitlab/bitbucket_server_import/importer.rb @@ -133,7 +133,10 @@ module Gitlab log_info(stage: 'import_repository', message: 'finished import') rescue Gitlab::Shell::Error => e - log_error(stage: 'import_repository', message: 'failed import', error: e.message) + Gitlab::Sentry.log_exception( + e, + stage: 'import_repository', message: 'failed import', error: e.message + ) # Expire cache to prevent scenarios such as: # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true @@ -164,8 +167,10 @@ module Gitlab batch.each do |pull_request| import_bitbucket_pull_request(pull_request) rescue StandardError => e - backtrace = Gitlab::Profiler.clean_backtrace(e.backtrace) - log_error(stage: 'import_pull_requests', iid: pull_request.iid, error: e.message, backtrace: backtrace) + Gitlab::Sentry.log_exception( + e, + stage: 'import_pull_requests', iid: pull_request.iid, error: e.message + ) errors << { type: :pull_request, iid: pull_request.iid, errors: e.message, backtrace: backtrace.join("\n"), raw_response: pull_request.raw } end @@ -177,7 +182,11 @@ module Gitlab client.delete_branch(project_key, repository_slug, branch.name, branch.sha) project.repository.delete_branch(branch.name) rescue BitbucketServer::Connection::ConnectionError => e - log_error(stage: 'delete_temp_branches', branch: branch.name, error: e.message) + Gitlab::Sentry.log_exception( + e, + stage: 'delete_temp_branches', branch: branch.name, error: e.message + ) + @errors << { type: :delete_temp_branches, branch_name: branch.name, errors: e.message } end end @@ -288,7 +297,11 @@ module Gitlab # a regular note. create_fallback_diff_note(merge_request, comment, position) rescue StandardError => e - log_error(stage: 'create_diff_note', comment_id: comment.id, error: e.message) + Gitlab::Sentry.log_exception( + e, + stage: 'create_diff_note', comment_id: comment.id, error: e.message + ) + errors << { type: :pull_request, id: comment.id, errors: e.message } nil end @@ -325,7 +338,11 @@ module Gitlab merge_request.notes.create!(pull_request_comment_attributes(replies)) end rescue StandardError => e - log_error(stage: 'import_standalone_pr_comments', merge_request_id: merge_request.id, comment_id: comment.id, error: e.message) + Gitlab::Sentry.log_exception( + e, + stage: 'import_standalone_pr_comments', merge_request_id: merge_request.id, comment_id: comment.id, error: e.message + ) + errors << { type: :pull_request, comment_id: comment.id, errors: e.message } end end @@ -360,10 +377,6 @@ module Gitlab logger.info(log_base_data.merge(details)) end - def log_error(details) - logger.error(log_base_data.merge(details)) - end - def log_warn(details) logger.warn(log_base_data.merge(details)) end diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 9c1e6277e95..ef11b8dc530 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -67,11 +67,11 @@ module Gitlab build_config(config) rescue Gitlab::Config::Loader::Yaml::DataTooLargeError => e - track_exception(e) + track_and_raise_for_dev_exception(e) raise Config::ConfigError, e.message rescue Gitlab::Ci::Config::External::Context::TimeoutError => e - track_exception(e) + track_and_raise_for_dev_exception(e) raise Config::ConfigError, TIMEOUT_MESSAGE end @@ -94,8 +94,8 @@ module Gitlab user: user) end - def track_exception(error) - Gitlab::Sentry.track_exception(error, extra: @context.sentry_payload) + def track_and_raise_for_dev_exception(error) + Gitlab::Sentry.track_and_raise_for_dev_exception(error, @context.sentry_payload) end # Overriden in EE diff --git a/lib/gitlab/ci/pipeline/chain/config/process.rb b/lib/gitlab/ci/pipeline/chain/config/process.rb index 731b0fdb286..feb5b4556e1 100644 --- a/lib/gitlab/ci/pipeline/chain/config/process.rb +++ b/lib/gitlab/ci/pipeline/chain/config/process.rb @@ -21,10 +21,10 @@ module Gitlab rescue Gitlab::Ci::YamlProcessor::ValidationError => ex error(ex.message, config_error: true) rescue => ex - Gitlab::Sentry.track_acceptable_exception(ex, extra: { + Gitlab::Sentry.track_exception(ex, project_id: project.id, sha: @pipeline.sha - }) + ) error("Undefined error (#{Labkit::Correlation::CorrelationId.current_id})", config_error: true) diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index ca7974930af..676743dbd59 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -35,7 +35,7 @@ module Gitlab # match the blob, which is a bug. But we shouldn't fail to render # completely in that case, even though we want to report the error. rescue RangeError => e - Gitlab::Sentry.track_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/45441') + Gitlab::Sentry.track_and_raise_for_dev_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/45441') end end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 373539f5516..41207e8cbde 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -67,8 +67,7 @@ module Gitlab File.read(cert_file).scan(PEM_REGEX).map do |cert| OpenSSL::X509::Certificate.new(cert).to_pem rescue OpenSSL::OpenSSLError => e - Rails.logger.error "Could not load certificate #{cert_file} #{e}" # rubocop:disable Gitlab/RailsLogger - Gitlab::Sentry.track_exception(e, extra: { cert_file: cert_file }) + Gitlab::Sentry.track_and_raise_for_dev_exception(e, cert_file: cert_file) nil end.compact end.uniq.join("\n") diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index dc15b95505a..d762ea179e3 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -91,12 +91,10 @@ module Gitlab project.repository.add_branch(project.creator, source_branch, pull_request.source_branch_sha) rescue Gitlab::Git::CommandError => e - Gitlab::Sentry.track_acceptable_exception(e, - extra: { - source_branch: source_branch, - project_id: merge_request.project.id, - merge_request_id: merge_request.id - }) + Gitlab::Sentry.track_exception(e, + source_branch: source_branch, + project_id: merge_request.project.id, + merge_request_id: merge_request.id) end end end diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb index abe90bba19c..048534e04de 100644 --- a/lib/gitlab/gpg.rb +++ b/lib/gitlab/gpg.rb @@ -110,9 +110,9 @@ module Gitlab folder_contents = Dir.children(tmp_dir) # This means we left a GPG-agent process hanging. Logging the problem in # sentry will make this more visible. - Gitlab::Sentry.track_exception(e, + Gitlab::Sentry.track_and_raise_for_dev_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918', - extra: { tmp_dir: tmp_dir, contents: folder_contents }) + tmp_dir: tmp_dir, contents: folder_contents) end tmp_keychains_removed.increment unless File.exist?(tmp_dir) diff --git a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb index fbd5e348c7d..118748d5a56 100644 --- a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb +++ b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb @@ -32,7 +32,7 @@ module Gitlab # Will inform you if there needs to be `calls_gitaly: true` as a kwarg in the field declaration # if there is at least 1 Gitaly call involved with the field resolution. error = RuntimeError.new("Gitaly is called for field '#{type_object.name}' on #{type_object.owner.try(:name)} - please either specify a constant complexity or add `calls_gitaly: true` to the field declaration") - Gitlab::Sentry.track_exception(error) + Gitlab::Sentry.track_and_raise_for_dev_exception(error) end end end diff --git a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb index 01b55a1667f..a6eb35be427 100644 --- a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb +++ b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb @@ -18,7 +18,7 @@ module Gitlab variables: variables }) rescue => e - Gitlab::Sentry.track_exception(e) + Gitlab::Sentry.track_and_raise_for_dev_exception(e) default_initial_values(query) end @@ -38,7 +38,7 @@ module Gitlab GraphqlLogger.info(memo.except!(:time_started, :query)) rescue => e - Gitlab::Sentry.track_exception(e) + Gitlab::Sentry.track_and_raise_for_dev_exception(e) end private diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb index 5663b9f20cf..fdb81e62bd7 100644 --- a/lib/gitlab/highlight.rb +++ b/lib/gitlab/highlight.rb @@ -61,7 +61,7 @@ module Gitlab tokens = lexer.lex(text, continue: continue) Timeout.timeout(timeout_time) { @formatter.format(tokens, tag: tag).html_safe } rescue Timeout::Error => e - Gitlab::Sentry.track_exception(e) + Gitlab::Sentry.track_and_raise_for_dev_exception(e) highlight_plain(text) rescue highlight_plain(text) diff --git a/lib/gitlab/import_export/relation_tree_restorer.rb b/lib/gitlab/import_export/relation_tree_restorer.rb index 8a2b0cab915..d2d0cddfcbb 100644 --- a/lib/gitlab/import_export/relation_tree_restorer.rb +++ b/lib/gitlab/import_export/relation_tree_restorer.rb @@ -82,11 +82,8 @@ module Gitlab end def log_import_failure(relation_key, relation_index, exception) - Gitlab::Sentry.track_acceptable_exception( - exception, - extra: { project_id: @importable.id, - relation_key: relation_key, - relation_index: relation_index }) + Gitlab::Sentry.track_exception(exception, + project_id: @importable.id, relation_key: relation_key, relation_index: relation_index) ImportFailure.create( project: @importable, diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 2539a6828c3..7bdfd928723 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -56,11 +56,7 @@ module Gitlab end def error(error) - error_payload = { message: error.message } - error_payload[:error_backtrace] = Gitlab::Profiler.clean_backtrace(error.backtrace) if error.backtrace - log_error(error_payload) - - Gitlab::Sentry.track_acceptable_exception(error, extra: log_base_data) + Gitlab::Sentry.track_exception(error, log_base_data) add_error_message(error.message) end diff --git a/lib/gitlab/profiler.rb b/lib/gitlab/profiler.rb index 560618bb486..f2f6180c464 100644 --- a/lib/gitlab/profiler.rb +++ b/lib/gitlab/profiler.rb @@ -118,6 +118,8 @@ module Gitlab end def self.clean_backtrace(backtrace) + return unless backtrace + Array(Rails.backtrace_cleaner.clean(backtrace)).reject do |line| line.match(Regexp.union(IGNORE_BACKTRACES)) end diff --git a/lib/gitlab/sentry.rb b/lib/gitlab/sentry.rb index 005cb3112b8..aa8f6fa12b1 100644 --- a/lib/gitlab/sentry.rb +++ b/lib/gitlab/sentry.rb @@ -2,76 +2,153 @@ module Gitlab module Sentry - def self.enabled? - (Rails.env.production? || Rails.env.development?) && - Gitlab.config.sentry.enabled - end + class << self + def configure + Raven.configure do |config| + config.dsn = sentry_dsn + config.release = Gitlab.revision + config.current_environment = Gitlab.config.sentry.environment + + # Sanitize fields based on those sanitized from Rails. + config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s) + # Sanitize authentication headers + config.sanitize_http_headers = %w[Authorization Private-Token] + config.tags = { program: Gitlab.process_name } + # Debugging for https://gitlab.com/gitlab-org/gitlab-foss/issues/57727 + config.before_send = method(:add_context_from_exception_type) + end + end + + def with_context(current_user = nil) + last_user_context = Raven.context.user - def self.context(current_user = nil) - return unless enabled? + user_context = { + id: current_user&.id, + email: current_user&.email, + username: current_user&.username + }.compact - Raven.tags_context(default_tags) + Raven.tags_context(default_tags) + Raven.user_context(user_context) - if current_user - Raven.user_context( - id: current_user.id, - email: current_user.email, - username: current_user.username - ) + yield + ensure + Raven.user_context(last_user_context) end - end - # This can be used for investigating exceptions that can be recovered from in - # code. The exception will still be raised in development and test - # environments. - # - # That way we can track down these exceptions with as much information as we - # need to resolve them. - # - # Provide an issue URL for follow up. - def self.track_exception(exception, issue_url: nil, extra: {}) - track_acceptable_exception(exception, issue_url: issue_url, extra: extra) - - raise exception if should_raise_for_dev? - end + # This should be used when you want to passthrough exception handling: + # rescue and raise to be catched in upper layers of the application. + # + # If the exception implements the method `sentry_extra_data` and that method + # returns a Hash, then the return value of that method will be merged into + # `extra`. Exceptions can use this mechanism to provide structured data + # to sentry in addition to their message and back-trace. + def track_and_raise_exception(exception, extra = {}) + process_exception(exception, sentry: true, extra: extra) - # This should be used when you do not want to raise an exception in - # development and test. If you need development and test to behave - # just the same as production you can use this instead of - # track_exception. - # - # If the exception implements the method `sentry_extra_data` and that method - # returns a Hash, then the return value of that method will be merged into - # `extra`. Exceptions can use this mechanism to provide structured data - # to sentry in addition to their message and back-trace. - def self.track_acceptable_exception(exception, issue_url: nil, extra: {}) - if enabled? - extra = build_extra_data(exception, issue_url, extra) - context # Make sure we've set everything we know in the context - - Raven.capture_exception(exception, tags: default_tags, extra: extra) + raise exception end - end - def self.should_raise_for_dev? - Rails.env.development? || Rails.env.test? - end + # This can be used for investigating exceptions that can be recovered from in + # code. The exception will still be raised in development and test + # environments. + # + # That way we can track down these exceptions with as much information as we + # need to resolve them. + # + # If the exception implements the method `sentry_extra_data` and that method + # returns a Hash, then the return value of that method will be merged into + # `extra`. Exceptions can use this mechanism to provide structured data + # to sentry in addition to their message and back-trace. + # + # Provide an issue URL for follow up. + # as `issue_url: 'http://gitlab.com/gitlab-org/gitlab/issues/111'` + def track_and_raise_for_dev_exception(exception, extra = {}) + process_exception(exception, sentry: true, extra: extra) - def self.default_tags - { - Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id, - locale: I18n.locale - } - end + raise exception if should_raise_for_dev? + end - def self.build_extra_data(exception, issue_url, extra) - exception.try(:sentry_extra_data)&.tap do |data| - extra.merge!(data) if data.is_a?(Hash) + # This should be used when you only want to track the exception. + # + # If the exception implements the method `sentry_extra_data` and that method + # returns a Hash, then the return value of that method will be merged into + # `extra`. Exceptions can use this mechanism to provide structured data + # to sentry in addition to their message and back-trace. + def track_exception(exception, extra = {}) + process_exception(exception, sentry: true, extra: extra) end - extra.merge({ issue_url: issue_url }.compact) - end + # This should be used when you only want to log the exception, + # but not send it to Sentry. + # + # If the exception implements the method `sentry_extra_data` and that method + # returns a Hash, then the return value of that method will be merged into + # `extra`. Exceptions can use this mechanism to provide structured data + # to sentry in addition to their message and back-trace. + def log_exception(exception, extra = {}) + process_exception(exception, extra: extra) + end + + private + + def process_exception(exception, sentry: false, logging: true, extra:) + exception.try(:sentry_extra_data)&.tap do |data| + extra = extra.merge(data) if data.is_a?(Hash) + end + + if sentry && Raven.configuration.server + Raven.capture_exception(exception, tags: default_tags, extra: extra) + end - private_class_method :build_extra_data + if logging + # TODO: this logic could migrate into `Gitlab::ExceptionLogFormatter` + # and we could also flatten deep nested hashes if required for search + # (e.g. if `extra` includes hash of hashes). + # In the current implementation, we don't flatten multi-level folded hashes. + log_hash = {} + Raven.context.tags.each { |name, value| log_hash["tags.#{name}"] = value } + Raven.context.user.each { |name, value| log_hash["user.#{name}"] = value } + Raven.context.extra.merge(extra).each { |name, value| log_hash["extra.#{name}"] = value } + + Gitlab::ExceptionLogFormatter.format!(exception, log_hash) + + Gitlab::Sentry::Logger.error(log_hash) + end + end + + def sentry_dsn + return unless Rails.env.production? || Rails.env.development? + return unless Gitlab.config.sentry.enabled + + Gitlab.config.sentry.dsn + end + + def should_raise_for_dev? + Rails.env.development? || Rails.env.test? + end + + def default_tags + { + Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id, + locale: I18n.locale + } + end + + def add_context_from_exception_type(event, hint) + if ActiveModel::MissingAttributeError === hint[:exception] + columns_hash = ActiveRecord::Base + .connection + .schema_cache + .instance_variable_get(:@columns_hash) + .map { |k, v| [k, v.map(&:first)] } + .to_h + + event.extra.merge!(columns_hash) + end + + event + end + end end end diff --git a/lib/gitlab/sentry/logger.rb b/lib/gitlab/sentry/logger.rb new file mode 100644 index 00000000000..fa24b8d17d2 --- /dev/null +++ b/lib/gitlab/sentry/logger.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Gitlab + module Sentry + class Logger < ::Gitlab::JsonLogger + def self.file_name_noext + 'exceptions_json' + end + end + end +end diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 28e5d0ba8f5..45de77f77aa 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -126,7 +126,7 @@ module Gitlab true rescue => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { path: path, new_path: new_path, storage: storage }) + Gitlab::Sentry.track_exception(e, path: path, new_path: new_path, storage: storage) false end @@ -158,7 +158,7 @@ module Gitlab true rescue => e Rails.logger.warn("Repository does not exist: #{e} at: #{name}.git") # rubocop:disable Gitlab/RailsLogger - Gitlab::Sentry.track_acceptable_exception(e, extra: { path: name, storage: storage }) + Gitlab::Sentry.track_exception(e, path: name, storage: storage) false end @@ -267,7 +267,7 @@ module Gitlab def mv_namespace(storage, old_name, new_name) Gitlab::GitalyClient::NamespaceService.new(storage).rename(old_name, new_name) rescue GRPC::InvalidArgument => e - Gitlab::Sentry.track_acceptable_exception(e, extra: { old_name: old_name, new_name: new_name, storage: storage }) + Gitlab::Sentry.track_exception(e, old_name: old_name, new_name: new_name, storage: storage) false end diff --git a/lib/gitlab/usage_data_counters/base_counter.rb b/lib/gitlab/usage_data_counters/base_counter.rb index 2b52571c3cc..461d562a0d4 100644 --- a/lib/gitlab/usage_data_counters/base_counter.rb +++ b/lib/gitlab/usage_data_counters/base_counter.rb @@ -8,7 +8,7 @@ module Gitlab::UsageDataCounters class << self def redis_key(event) - Gitlab::Sentry.track_exception(UnknownEvent, extra: { event: event }) unless known_events.include?(event.to_s) + Gitlab::Sentry.track_and_raise_for_dev_exception(UnknownEvent.new, event: event) unless known_events.include?(event.to_s) "USAGE_#{prefix}_#{event}".upcase end diff --git a/lib/sentry/client.rb b/lib/sentry/client.rb index 47b497accc1..851896a6429 100644 --- a/lib/sentry/client.rb +++ b/lib/sentry/client.rb @@ -62,7 +62,7 @@ module Sentry def handle_mapping_exceptions(&block) yield rescue KeyError => e - Gitlab::Sentry.track_acceptable_exception(e) + Gitlab::Sentry.track_exception(e) raise MissingKeysError, "Sentry API response is missing keys. #{e.message}" end @@ -118,7 +118,7 @@ module Sentry def handle_request_exceptions yield rescue Gitlab::HTTP::Error => e - Gitlab::Sentry.track_acceptable_exception(e) + Gitlab::Sentry.track_exception(e) raise_error 'Error when connecting to Sentry' rescue Net::OpenTimeout raise_error 'Connection to Sentry timed out' @@ -129,7 +129,7 @@ module Sentry rescue Errno::ECONNREFUSED raise_error 'Connection refused' rescue => e - Gitlab::Sentry.track_acceptable_exception(e) + Gitlab::Sentry.track_exception(e) raise_error "Sentry request failed due to #{e.class}" end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 405d6948bad..28210dad9bb 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19211,9 +19211,6 @@ msgstr "" msgid "Upload file" msgstr "" -msgid "Upload file does not exist" -msgstr "" - msgid "Upload object map" msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index e4e2487f1e1..68b73150b31 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1159,7 +1159,7 @@ describe Projects::IssuesController do end it "prevents deletion if destroy_confirm is not set" do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } @@ -1168,7 +1168,7 @@ describe Projects::IssuesController do end it "prevents deletion in JSON format if destroy_confirm is not set" do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, format: 'json' } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index ef5f286502b..4b1304a9103 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -620,7 +620,7 @@ describe Projects::MergeRequestsController do end it "prevents deletion if destroy_confirm is not set" do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid } @@ -629,7 +629,7 @@ describe Projects::MergeRequestsController do end it "prevents deletion in JSON format if destroy_confirm is not set" do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, format: 'json' } diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js index 97f7f05cd85..0e818d6c402 100644 --- a/spec/frontend/lib/utils/url_utility_spec.js +++ b/spec/frontend/lib/utils/url_utility_spec.js @@ -323,3 +323,45 @@ describe('URL utility', () => { }); }); }); + +describe('setUrlParams', () => { + it('adds new params as query string', () => { + const url = 'https://gitlab.com/test'; + + expect( + urlUtils.setUrlParams({ group_id: 'gitlab-org', project_id: 'my-project' }, url), + ).toEqual('https://gitlab.com/test?group_id=gitlab-org&project_id=my-project'); + }); + + it('updates an existing parameter', () => { + const url = 'https://gitlab.com/test?group_id=gitlab-org&project_id=my-project'; + + expect(urlUtils.setUrlParams({ project_id: 'gitlab-test' }, url)).toEqual( + 'https://gitlab.com/test?group_id=gitlab-org&project_id=gitlab-test', + ); + }); + + it("removes the project_id param when it's value is null", () => { + const url = 'https://gitlab.com/test?group_id=gitlab-org&project_id=my-project'; + + expect(urlUtils.setUrlParams({ project_id: null }, url)).toEqual( + 'https://gitlab.com/test?group_id=gitlab-org', + ); + }); + + it('handles arrays properly', () => { + const url = 'https://gitlab.com/test'; + + expect(urlUtils.setUrlParams({ label_name: ['foo', 'bar'] }, url)).toEqual( + 'https://gitlab.com/test?label_name=foo&label_name=bar', + ); + }); + + it('removes all existing URL params and sets a new param when cleanParams=true', () => { + const url = 'https://gitlab.com/test?group_id=gitlab-org&project_id=my-project'; + + expect(urlUtils.setUrlParams({ foo: 'bar' }, url, true)).toEqual( + 'https://gitlab.com/test?foo=bar', + ); + }); +}); diff --git a/spec/frontend/monitoring/charts/time_series_spec.js b/spec/frontend/monitoring/charts/time_series_spec.js index 15967992374..128c4bc49f1 100644 --- a/spec/frontend/monitoring/charts/time_series_spec.js +++ b/spec/frontend/monitoring/charts/time_series_spec.js @@ -46,7 +46,10 @@ describe('Time series component', () => { store.commit(`monitoringDashboard/${types.RECEIVE_DEPLOYMENTS_DATA_SUCCESS}`, deploymentData); // Mock data contains 2 panel groups, with 1 and 2 panels respectively - store.commit(`monitoringDashboard/${types.SET_QUERY_RESULT}`, mockedQueryResultPayload); + store.commit( + `monitoringDashboard/${types.RECEIVE_METRIC_RESULT_SUCCESS}`, + mockedQueryResultPayload, + ); // Pick the second panel group and the first panel in it [mockGraphData] = store.state.monitoringDashboard.dashboard.panel_groups[1].panels; diff --git a/spec/frontend/monitoring/store/actions_spec.js b/spec/frontend/monitoring/store/actions_spec.js index e41158eb4b0..92d469270c9 100644 --- a/spec/frontend/monitoring/store/actions_spec.js +++ b/spec/frontend/monitoring/store/actions_spec.js @@ -434,13 +434,11 @@ describe('Monitoring store actions', () => { start: '2019-08-06T12:40:02.184Z', end: '2019-08-06T20:40:02.184Z', }; - let commit; let metric; let state; let data; beforeEach(() => { - commit = jest.fn(); state = storeState(); [metric] = metricsDashboardResponse.dashboard.panel_groups[0].panels[0].metrics; [data] = metricsGroupsAPIResponse[0].panels[0].metrics; @@ -449,17 +447,31 @@ describe('Monitoring store actions', () => { it('commits result', done => { mock.onGet('http://test').reply(200, { data }); // One attempt - fetchPrometheusMetric({ state, commit }, { metric, params }) - .then(() => { - expect(commit).toHaveBeenCalledWith(types.SET_QUERY_RESULT, { - metricId: metric.metric_id, - result: data.result, - }); - + testAction( + fetchPrometheusMetric, + { metric, params }, + state, + [ + { + type: types.REQUEST_METRIC_RESULT, + payload: { + metricId: metric.metric_id, + }, + }, + { + type: types.RECEIVE_METRIC_RESULT_SUCCESS, + payload: { + metricId: metric.metric_id, + result: data.result, + }, + }, + ], + [], + () => { expect(mock.history.get).toHaveLength(1); done(); - }) - .catch(done.fail); + }, + ).catch(done.fail); }); it('commits result, when waiting for results', done => { @@ -469,18 +481,31 @@ describe('Monitoring store actions', () => { mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT); mock.onGet('http://test').reply(200, { data }); // 4th attempt - const fetch = fetchPrometheusMetric({ state, commit }, { metric, params }); - - fetch - .then(() => { - expect(commit).toHaveBeenCalledWith(types.SET_QUERY_RESULT, { - metricId: metric.metric_id, - result: data.result, - }); + testAction( + fetchPrometheusMetric, + { metric, params }, + state, + [ + { + type: types.REQUEST_METRIC_RESULT, + payload: { + metricId: metric.metric_id, + }, + }, + { + type: types.RECEIVE_METRIC_RESULT_SUCCESS, + payload: { + metricId: metric.metric_id, + result: data.result, + }, + }, + ], + [], + () => { expect(mock.history.get).toHaveLength(4); done(); - }) - .catch(done.fail); + }, + ).catch(done.fail); }); it('commits failure, when waiting for results and getting a server error', done => { @@ -490,15 +515,33 @@ describe('Monitoring store actions', () => { mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT); mock.onGet('http://test').reply(500); // 4th attempt - fetchPrometheusMetric({ state, commit }, { metric, params }) - .then(() => { - done.fail(); - }) - .catch(() => { - expect(commit).not.toHaveBeenCalled(); - expect(mock.history.get).toHaveLength(4); - done(); - }); + const error = new Error('Request failed with status code 500'); + + testAction( + fetchPrometheusMetric, + { metric, params }, + state, + [ + { + type: types.REQUEST_METRIC_RESULT, + payload: { + metricId: metric.metric_id, + }, + }, + { + type: types.RECEIVE_METRIC_RESULT_ERROR, + payload: { + metricId: metric.metric_id, + error, + }, + }, + ], + [], + ).catch(e => { + expect(mock.history.get).toHaveLength(4); + expect(e).toEqual(error); + done(); + }); }); }); }); diff --git a/spec/frontend/monitoring/store/getters_spec.js b/spec/frontend/monitoring/store/getters_spec.js index 066f927dce7..3b6f33ed8b1 100644 --- a/spec/frontend/monitoring/store/getters_spec.js +++ b/spec/frontend/monitoring/store/getters_spec.js @@ -57,22 +57,22 @@ describe('Monitoring store Getters', () => { it('an empty metric, returns empty', () => { mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, metricsGroupsAPIResponse); - mutations[types.SET_QUERY_RESULT](state, mockedEmptyResult); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedEmptyResult); expect(metricsWithData()).toEqual([]); }); it('a metric with results, it returns a metric', () => { mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, metricsGroupsAPIResponse); - mutations[types.SET_QUERY_RESULT](state, mockedQueryResultPayload); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedQueryResultPayload); expect(metricsWithData()).toEqual([mockedQueryResultPayload.metricId]); }); it('multiple metrics with results, it return multiple metrics', () => { mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, metricsGroupsAPIResponse); - mutations[types.SET_QUERY_RESULT](state, mockedQueryResultPayload); - mutations[types.SET_QUERY_RESULT](state, mockedQueryResultPayloadCoresTotal); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedQueryResultPayload); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedQueryResultPayloadCoresTotal); expect(metricsWithData()).toEqual([ mockedQueryResultPayload.metricId, @@ -82,8 +82,8 @@ describe('Monitoring store Getters', () => { it('multiple metrics with results, it returns metrics filtered by group', () => { mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, metricsGroupsAPIResponse); - mutations[types.SET_QUERY_RESULT](state, mockedQueryResultPayload); - mutations[types.SET_QUERY_RESULT](state, mockedQueryResultPayloadCoresTotal); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedQueryResultPayload); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](state, mockedQueryResultPayloadCoresTotal); // First group has no metrics expect(metricsWithData(state.dashboard.panel_groups[0].key)).toEqual([]); diff --git a/spec/frontend/monitoring/store/mutations_spec.js b/spec/frontend/monitoring/store/mutations_spec.js index 47177180aa1..8da172ec634 100644 --- a/spec/frontend/monitoring/store/mutations_spec.js +++ b/spec/frontend/monitoring/store/mutations_spec.js @@ -1,6 +1,9 @@ +import httpStatusCodes from '~/lib/utils/http_status'; + import mutations from '~/monitoring/stores/mutations'; import * as types from '~/monitoring/stores/mutation_types'; import state from '~/monitoring/stores/state'; +import { metricsErrors } from '~/monitoring/constants'; import { metricsGroupsAPIResponse, deploymentData, @@ -90,7 +93,7 @@ describe('Monitoring mutations', () => { expect(stateCopy.projectPath).toEqual('/gitlab-org/gitlab-foss'); }); }); - describe('SET_QUERY_RESULT', () => { + describe('Individual panel/metric results', () => { const metricId = '12_system_metrics_kubernetes_container_memory_total'; const result = [ { @@ -98,31 +101,145 @@ describe('Monitoring mutations', () => { }, ]; const dashboardGroups = metricsDashboardResponse.dashboard.panel_groups; - const getMetrics = () => stateCopy.dashboard.panel_groups[0].panels[0].metrics; + const getMetric = () => stateCopy.dashboard.panel_groups[0].panels[0].metrics[0]; - beforeEach(() => { - mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + describe('REQUEST_METRIC_RESULT', () => { + beforeEach(() => { + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + }); + it('stores a loading state on a metric', () => { + expect(stateCopy.showEmptyState).toBe(true); + + mutations[types.REQUEST_METRIC_RESULT](stateCopy, { + metricId, + result, + }); + + expect(stateCopy.showEmptyState).toBe(true); + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: true, + result: null, + error: null, + }), + ); + }); }); - it('clears empty state', () => { - expect(stateCopy.showEmptyState).toBe(true); - mutations[types.SET_QUERY_RESULT](stateCopy, { - metricId, - result, + describe('RECEIVE_METRIC_RESULT_SUCCESS', () => { + beforeEach(() => { + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); }); + it('clears empty state', () => { + expect(stateCopy.showEmptyState).toBe(true); - expect(stateCopy.showEmptyState).toBe(false); + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](stateCopy, { + metricId, + result, + }); + + expect(stateCopy.showEmptyState).toBe(false); + }); + + it('adds results to the store', () => { + expect(getMetric().result).toBe(undefined); + + mutations[types.RECEIVE_METRIC_RESULT_SUCCESS](stateCopy, { + metricId, + result, + }); + + expect(getMetric().result).toHaveLength(result.length); + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: false, + error: null, + }), + ); + }); }); - it('adds results to the store', () => { - expect(getMetrics()[0].result).toBe(undefined); + describe('RECEIVE_METRIC_RESULT_ERROR', () => { + beforeEach(() => { + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + }); + it('maintains the loading state when a metric fails', () => { + expect(stateCopy.showEmptyState).toBe(true); + + mutations[types.RECEIVE_METRIC_RESULT_ERROR](stateCopy, { + metricId, + error: 'an error', + }); + + expect(stateCopy.showEmptyState).toBe(true); + }); + + it('stores a timeout error in a metric', () => { + mutations[types.RECEIVE_METRIC_RESULT_ERROR](stateCopy, { + metricId, + error: { message: 'BACKOFF_TIMEOUT' }, + }); + + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: false, + result: null, + error: metricsErrors.TIMEOUT, + }), + ); + }); + + it('stores a connection failed error in a metric', () => { + mutations[types.RECEIVE_METRIC_RESULT_ERROR](stateCopy, { + metricId, + error: { + response: { + status: httpStatusCodes.SERVICE_UNAVAILABLE, + }, + }, + }); + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: false, + result: null, + error: metricsErrors.CONNECTION_FAILED, + }), + ); + }); - mutations[types.SET_QUERY_RESULT](stateCopy, { - metricId, - result, + it('stores a bad data error in a metric', () => { + mutations[types.RECEIVE_METRIC_RESULT_ERROR](stateCopy, { + metricId, + error: { + response: { + status: httpStatusCodes.BAD_REQUEST, + }, + }, + }); + + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: false, + result: null, + error: metricsErrors.BAD_DATA, + }), + ); }); - expect(getMetrics()[0].result).toHaveLength(result.length); + it('stores an unknown error in a metric', () => { + mutations[types.RECEIVE_METRIC_RESULT_ERROR](stateCopy, { + metricId, + error: null, // no reason in response + }); + + expect(getMetric()).toEqual( + expect.objectContaining({ + loading: false, + result: null, + error: metricsErrors.UNKNOWN_ERROR, + }), + ); + }); }); }); describe('SET_ALL_DASHBOARDS', () => { diff --git a/spec/javascripts/monitoring/components/dashboard_spec.js b/spec/javascripts/monitoring/components/dashboard_spec.js index 7d6a1d7b097..37a811f153f 100644 --- a/spec/javascripts/monitoring/components/dashboard_spec.js +++ b/spec/javascripts/monitoring/components/dashboard_spec.js @@ -56,13 +56,16 @@ function setupComponentStore(component) { ); // Load 3 panels to the dashboard, one with an empty result - component.$store.commit(`monitoringDashboard/${types.SET_QUERY_RESULT}`, mockedEmptyResult); component.$store.commit( - `monitoringDashboard/${types.SET_QUERY_RESULT}`, + `monitoringDashboard/${types.RECEIVE_METRIC_RESULT_SUCCESS}`, + mockedEmptyResult, + ); + component.$store.commit( + `monitoringDashboard/${types.RECEIVE_METRIC_RESULT_SUCCESS}`, mockedQueryResultPayload, ); component.$store.commit( - `monitoringDashboard/${types.SET_QUERY_RESULT}`, + `monitoringDashboard/${types.RECEIVE_METRIC_RESULT_SUCCESS}`, mockedQueryResultPayloadCoresTotal, ); @@ -269,7 +272,7 @@ describe('Dashboard', () => { metricsGroupsAPIResponse, ); component.$store.commit( - `monitoringDashboard/${types.SET_QUERY_RESULT}`, + `monitoringDashboard/${types.RECEIVE_METRIC_RESULT_SUCCESS}`, mockedQueryResultPayload, ); diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 6594a3dfa8b..b039c572677 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -157,7 +157,7 @@ describe Gitlab::Ci::Config do describe '.new' do it 'raises error' do - expect(Gitlab::Sentry).to receive(:track_exception) + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) expect { config }.to raise_error( described_class::ConfigError, @@ -367,7 +367,7 @@ describe Gitlab::Ci::Config do end it 'raises error TimeoutError' do - expect(Gitlab::Sentry).to receive(:track_exception) + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) expect { config }.to raise_error( described_class::ConfigError, diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index f5d3d14ccc5..1a14f6d4f67 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -105,7 +105,7 @@ describe Gitlab::Diff::Highlight do end it 'keeps the original rich line' do - allow(Gitlab::Sentry).to receive(:track_exception) + allow(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) code = %q{+ raise RuntimeError, "System commands must be given as an array of strings"} @@ -114,7 +114,7 @@ describe Gitlab::Diff::Highlight do end it 'reports to Sentry if configured' do - expect(Gitlab::Sentry).to receive(:track_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception).and_call_original expect { subject }. to raise_exception(RangeError) end diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index 80c1493d01b..b7ad6cebf81 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -86,12 +86,11 @@ describe Gitlab::GitalyClient do describe '.stub_certs' do it 'skips certificates if OpenSSLError is raised and report it' do - expect(Rails.logger).to receive(:error).at_least(:once) expect(Gitlab::Sentry) - .to receive(:track_exception) + .to receive(:track_and_raise_for_dev_exception) .with( a_kind_of(OpenSSL::X509::CertificateError), - extra: { cert_file: a_kind_of(String) }).at_least(:once) + cert_file: a_kind_of(String)).at_least(:once) expect(OpenSSL::X509::Certificate) .to receive(:new) diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 30d23fcf9d4..7065b3f9bcb 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -301,7 +301,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi it 'ignores Git command errors when creating a branch' do expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError) - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original mr = insert_git_data diff --git a/spec/lib/gitlab/gpg_spec.rb b/spec/lib/gitlab/gpg_spec.rb index 5d43023502c..c073d86b4a5 100644 --- a/spec/lib/gitlab/gpg_spec.rb +++ b/spec/lib/gitlab/gpg_spec.rb @@ -182,17 +182,17 @@ describe Gitlab::Gpg do expected_tmp_dir = nil expect(described_class).to receive(:cleanup_tmp_dir).and_raise(expected_exception) - allow(Gitlab::Sentry).to receive(:track_exception) + allow(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) described_class.using_tmp_keychain do expected_tmp_dir = described_class.current_home_dir FileUtils.touch(File.join(expected_tmp_dir, 'dummy.file')) end - expect(Gitlab::Sentry).to have_received(:track_exception).with( + expect(Gitlab::Sentry).to have_received(:track_and_raise_for_dev_exception).with( expected_exception, issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918', - extra: { tmp_dir: expected_tmp_dir, contents: ['dummy.file'] } + tmp_dir: expected_tmp_dir, contents: ['dummy.file'] ) end diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb index 8d522252e2d..9a2339e576a 100644 --- a/spec/lib/gitlab/import_export/shared_spec.rb +++ b/spec/lib/gitlab/import_export/shared_spec.rb @@ -49,24 +49,9 @@ describe Gitlab::ImportExport::Shared do it 'updates the import JID' do import_state = create(:import_state, project: project, jid: 'jid-test') - expect_next_instance_of(Gitlab::Import::Logger) do |logger| - expect(logger).to receive(:error).with(hash_including(import_jid: import_state.jid)) - end - - subject.error(error) - end - - it 'calls the error logger without a backtrace' do - expect(subject).to receive(:log_error).with(message: error.message) - - subject.error(error) - end - - it 'calls the error logger with the full message' do - backtrace = caller - allow(error).to receive(:backtrace).and_return(caller) - - expect(subject).to receive(:log_error).with(message: error.message, error_backtrace: Gitlab::Profiler.clean_backtrace(backtrace)) + expect(Gitlab::Sentry) + .to receive(:track_exception) + .with(error, hash_including(import_jid: import_state.jid)) subject.error(error) end diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb index 6a626d864a9..befbd1b4c19 100644 --- a/spec/lib/gitlab/import_export/version_checker_spec.rb +++ b/spec/lib/gitlab/import_export/version_checker_spec.rb @@ -8,10 +8,20 @@ describe Gitlab::ImportExport::VersionChecker do describe 'bundle a project Git repo' do let(:version) { Gitlab::ImportExport.version } + let(:version_file) { Tempfile.new('VERSION') } before do allow_any_instance_of(Gitlab::ImportExport::Shared).to receive(:relative_archive_path).and_return('') - allow(File).to receive(:open).and_return(version) + + version_file.write(version) + version_file.rewind + + allow_any_instance_of(described_class).to receive(:version_file).and_return(version_file.path) + end + + after do + version_file.close + version_file.unlink end it 'returns true if Import/Export have the same version' do diff --git a/spec/lib/gitlab/sentry_spec.rb b/spec/lib/gitlab/sentry_spec.rb index 024ac733a07..6d05241e72d 100644 --- a/spec/lib/gitlab/sentry_spec.rb +++ b/spec/lib/gitlab/sentry_spec.rb @@ -3,12 +3,31 @@ require 'spec_helper' describe Gitlab::Sentry do - describe '.context' do - it 'adds the expected tags' do - expect(described_class).to receive(:enabled?).and_return(true) - allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + let(:exception) { RuntimeError.new('boom') } + let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } + + let(:expected_payload_includes) do + [ + { 'exception.class' => 'RuntimeError' }, + { 'exception.message' => 'boom' }, + { 'tags.correlation_id' => 'cid' }, + { 'extra.some_other_info' => 'info' }, + { 'extra.issue_url' => 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } + ] + end + + before do + stub_sentry_settings - described_class.context(nil) + allow(described_class).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn) + allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + + described_class.configure + end + + describe '.with_context' do + it 'adds the expected tags' do + described_class.with_context {} expect(Raven.tags_context[:locale].to_s).to eq(I18n.locale.to_s) expect(Raven.tags_context[Labkit::Correlation::CorrelationId::LOG_KEY.to_sym].to_s) @@ -16,23 +35,23 @@ describe Gitlab::Sentry do end end - describe '.track_exception' do - let(:exception) { RuntimeError.new('boom') } + describe '.track_and_raise_for_dev_exception' do + context 'when exceptions for dev should be raised' do + before do + expect(described_class).to receive(:should_raise_for_dev?).and_return(true) + end - before do - allow(described_class).to receive(:enabled?).and_return(true) - end + it 'raises the exception' do + expect(Raven).to receive(:capture_exception) - it 'raises the exception if it should' do - expect(described_class).to receive(:should_raise_for_dev?).and_return(true) - expect { described_class.track_exception(exception) } - .to raise_error(RuntimeError) + expect { described_class.track_and_raise_for_dev_exception(exception) } + .to raise_error(RuntimeError) + end end - context 'when exceptions should not be raised' do + context 'when exceptions for dev should not be raised' do before do - allow(described_class).to receive(:should_raise_for_dev?).and_return(false) - allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + expect(described_class).to receive(:should_raise_for_dev?).and_return(false) end it 'logs the exception with all attributes passed' do @@ -50,30 +69,49 @@ describe Gitlab::Sentry do tags: a_hash_including(expected_tags), extra: a_hash_including(expected_extras)) - described_class.track_exception( + described_class.track_and_raise_for_dev_exception( exception, - issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1', - extra: { some_other_info: 'info' } + issue_url: issue_url, + some_other_info: 'info' ) end - it 'sets the context' do - expect(described_class).to receive(:context) + it 'calls Gitlab::Sentry::Logger.error with formatted payload' do + expect(Gitlab::Sentry::Logger).to receive(:error) + .with(a_hash_including(*expected_payload_includes)) - described_class.track_exception(exception) + described_class.track_and_raise_for_dev_exception( + exception, + issue_url: issue_url, + some_other_info: 'info' + ) end end end - context '.track_acceptable_exception' do - let(:exception) { RuntimeError.new('boom') } - let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } + describe '.track_and_raise_exception' do + it 'always raises the exception' do + expect(Raven).to receive(:capture_exception) + + expect { described_class.track_and_raise_exception(exception) } + .to raise_error(RuntimeError) + end - before do - allow(described_class).to receive(:enabled?).and_return(true) - allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + it 'calls Gitlab::Sentry::Logger.error with formatted payload' do + expect(Gitlab::Sentry::Logger).to receive(:error) + .with(a_hash_including(*expected_payload_includes)) + + expect do + described_class.track_and_raise_exception( + exception, + issue_url: issue_url, + some_other_info: 'info' + ) + end.to raise_error(RuntimeError) end + end + describe '.track_exception' do it 'calls Raven.capture_exception' do expected_extras = { some_other_info: 'info', @@ -89,34 +127,45 @@ describe Gitlab::Sentry do tags: a_hash_including(expected_tags), extra: a_hash_including(expected_extras)) - described_class.track_acceptable_exception( + described_class.track_exception( + exception, + issue_url: issue_url, + some_other_info: 'info' + ) + end + + it 'calls Gitlab::Sentry::Logger.error with formatted payload' do + expect(Gitlab::Sentry::Logger).to receive(:error) + .with(a_hash_including(*expected_payload_includes)) + + described_class.track_exception( exception, issue_url: issue_url, - extra: { some_other_info: 'info' } + some_other_info: 'info' ) end context 'the exception implements :sentry_extra_data' do let(:extra_info) { { event: 'explosion', size: :massive } } - let(:exception) { double(message: 'bang!', sentry_extra_data: extra_info) } + let(:exception) { double(message: 'bang!', sentry_extra_data: extra_info, backtrace: caller) } it 'includes the extra data from the exception in the tracking information' do expect(Raven).to receive(:capture_exception) .with(exception, a_hash_including(extra: a_hash_including(extra_info))) - described_class.track_acceptable_exception(exception) + described_class.track_exception(exception) end end context 'the exception implements :sentry_extra_data, which returns nil' do - let(:exception) { double(message: 'bang!', sentry_extra_data: nil) } + let(:exception) { double(message: 'bang!', sentry_extra_data: nil, backtrace: caller) } it 'just includes the other extra info' do extra_info = { issue_url: issue_url } expect(Raven).to receive(:capture_exception) .with(exception, a_hash_including(extra: a_hash_including(extra_info))) - described_class.track_acceptable_exception(exception, extra_info) + described_class.track_exception(exception, extra_info) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index bcab621ab6a..dde37e62e6c 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3522,7 +3522,7 @@ describe Ci::Build do end it 'can drop the build' do - expect(Gitlab::Sentry).to receive(:track_exception) + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) expect { build.drop! }.not_to raise_error diff --git a/spec/models/clusters/cluster_spec.rb b/spec/models/clusters/cluster_spec.rb index 01abaea7399..036b6c7902f 100644 --- a/spec/models/clusters/cluster_spec.rb +++ b/spec/models/clusters/cluster_spec.rb @@ -1004,8 +1004,8 @@ describe Clusters::Cluster, :use_clean_rails_memory_store_caching do it { is_expected.to eq(connection_status: :unknown_failure) } it 'notifies Sentry' do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception) - .with(instance_of(StandardError), hash_including(extra: { cluster_id: cluster.id })) + expect(Gitlab::Sentry).to receive(:track_exception) + .with(instance_of(StandardError), hash_including(cluster_id: cluster.id)) subject end diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb index 192e884f3e8..6af57283a2a 100644 --- a/spec/models/concerns/group_descendant_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -82,7 +82,7 @@ describe GroupDescendant do end it 'tracks the exception when a parent was not preloaded' do - expect(Gitlab::Sentry).to receive(:track_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception).and_call_original expect { described_class.build_hierarchy([subsub_group]) }.to raise_error(ArgumentError) end @@ -91,7 +91,7 @@ describe GroupDescendant do expected_hierarchy = { parent => { subgroup => subsub_group } } # this does not raise in production, so stubbing it here. - allow(Gitlab::Sentry).to receive(:track_exception) + allow(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) expect(described_class.build_hierarchy([subsub_group])).to eq(expected_hierarchy) end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 03434c95218..6128e7d2a24 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -171,8 +171,7 @@ describe Upload do it 'sends a message to Sentry' do upload = create(:upload, :issuable_upload) - expect(Gitlab::Sentry).to receive(:enabled?).and_return(true) - expect(Raven).to receive(:capture_message).with("Upload file does not exist", extra: upload.attributes) + expect(Gitlab::Sentry).to receive(:track_exception).with(instance_of(RuntimeError), upload.attributes) upload.exist? end diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 54401ec4085..bfdfd034cca 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -46,7 +46,7 @@ describe 'GraphQL' do 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::Sentry).to receive(:track_and_raise_for_dev_exception).at_least(1).times expect(Gitlab::GraphqlLogger).to receive(:info) post_graphql(query, variables: {}) @@ -146,7 +146,7 @@ describe 'GraphQL' do end it "logs a warning that the 'calls_gitaly' field declaration is missing" do - expect(Gitlab::Sentry).to receive(:track_exception).once + expect(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception).once post_graphql(query, current_user: user) end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 0c53c04ba40..1bec860bf0e 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -226,11 +226,11 @@ describe API::Helpers do describe '.handle_api_exception' do before do allow_any_instance_of(self.class).to receive(:rack_response) - allow(Gitlab::Sentry).to receive(:enabled?).and_return(true) stub_sentry_settings - configure_sentry + expect(Gitlab::Sentry).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn) + Gitlab::Sentry.configure Raven.client.configuration.encoding = 'json' end diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index 47bc26c0521..64fa74ccce5 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -60,10 +60,10 @@ describe Ci::ArchiveTraceService, '#execute' do it 'increments Prometheus counter, sends crash report to Sentry and ignore an error for continuing to archive' do expect(Gitlab::Sentry) - .to receive(:track_exception) + .to receive(:track_and_raise_for_dev_exception) .with(::Gitlab::Ci::Trace::ArchiveError, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/51502', - extra: { job_id: job.id } ).once + job_id: job.id).once expect(Sidekiq.logger).to receive(:warn).with( class: ArchiveTraceWorker.name, diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 4c83f9229d9..c4274f0bd17 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -528,7 +528,7 @@ describe Ci::CreatePipelineService do end it 'logs error' do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original execute_service end @@ -613,7 +613,7 @@ describe Ci::CreatePipelineService do end it 'logs error' do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original + expect(Gitlab::Sentry).to receive(:track_exception).and_call_original execute_service end diff --git a/spec/services/ci/prepare_build_service_spec.rb b/spec/services/ci/prepare_build_service_spec.rb index 2d027f13e52..87061b3b15a 100644 --- a/spec/services/ci/prepare_build_service_spec.rb +++ b/spec/services/ci/prepare_build_service_spec.rb @@ -51,8 +51,8 @@ describe Ci::PrepareBuildService do it 'drops the build and notifies Sentry' do expect(build).to receive(:drop).with(:unmet_prerequisites).once - expect(Gitlab::Sentry).to receive(:track_acceptable_exception) - .with(instance_of(Kubeclient::HttpError), hash_including(extra: { build_id: build.id })) + expect(Gitlab::Sentry).to receive(:track_exception) + .with(instance_of(Kubeclient::HttpError), hash_including(build_id: build.id)) subject end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 04334fb8915..aa31d98c4fb 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -514,8 +514,8 @@ module Ci subject { execute(specific_runner, {}) } it 'does drop the build and logs both failures' do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception) - .with(anything, a_hash_including(extra: a_hash_including(build_id: pending_job.id))) + expect(Gitlab::Sentry).to receive(:track_exception) + .with(anything, a_hash_including(build_id: pending_job.id)) .twice .and_call_original @@ -540,8 +540,8 @@ module Ci subject { execute(specific_runner, {}) } it 'does drop the build and logs failure' do - expect(Gitlab::Sentry).to receive(:track_acceptable_exception) - .with(anything, a_hash_including(extra: a_hash_including(build_id: pending_job.id))) + expect(Gitlab::Sentry).to receive(:track_exception) + .with(anything, a_hash_including(build_id: pending_job.id)) .once .and_call_original diff --git a/spec/support/shared_examples/services/base_helm_service_shared_examples.rb b/spec/support/shared_examples/services/base_helm_service_shared_examples.rb index fa76b95f768..f867cb69cfd 100644 --- a/spec/support/shared_examples/services/base_helm_service_shared_examples.rb +++ b/spec/support/shared_examples/services/base_helm_service_shared_examples.rb @@ -11,20 +11,10 @@ shared_examples 'logs kubernetes errors' do } end - let(:logger_hash) do - error_hash.merge( - exception: error_name, - message: error_message, - backtrace: instance_of(Array) - ) - end - it 'logs into kubernetes.log and Sentry' do - expect(service.send(:logger)).to receive(:error).with(hash_including(logger_hash)) - - expect(Gitlab::Sentry).to receive(:track_acceptable_exception).with( + expect(Gitlab::Sentry).to receive(:track_exception).with( error, - extra: hash_including(error_hash) + hash_including(error_hash) ) service.execute diff --git a/spec/workers/ci/archive_traces_cron_worker_spec.rb b/spec/workers/ci/archive_traces_cron_worker_spec.rb index 01232e2a58b..291fe54c4e3 100644 --- a/spec/workers/ci/archive_traces_cron_worker_spec.rb +++ b/spec/workers/ci/archive_traces_cron_worker_spec.rb @@ -63,7 +63,7 @@ describe Ci::ArchiveTracesCronWorker do let!(:build) { create(:ci_build, :success, :trace_live, finished_at: finished_at) } before do - allow(Gitlab::Sentry).to receive(:track_exception) + allow(Gitlab::Sentry).to receive(:track_and_raise_for_dev_exception) allow_any_instance_of(Gitlab::Ci::Trace).to receive(:archive!).and_raise('Unexpected error') end diff --git a/spec/workers/run_pipeline_schedule_worker_spec.rb b/spec/workers/run_pipeline_schedule_worker_spec.rb index 2bf1d470b09..1d376e1f02a 100644 --- a/spec/workers/run_pipeline_schedule_worker_spec.rb +++ b/spec/workers/run_pipeline_schedule_worker_spec.rb @@ -43,10 +43,10 @@ describe RunPipelineScheduleWorker do allow(Ci::CreatePipelineService).to receive(:new) { raise ActiveRecord::StatementInvalid } expect(Gitlab::Sentry) - .to receive(:track_exception) + .to receive(:track_and_raise_for_dev_exception) .with(ActiveRecord::StatementInvalid, issue_url: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/41231', - extra: { schedule_id: pipeline_schedule.id } ).once + schedule_id: pipeline_schedule.id).once end it 'increments Prometheus counter' do diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index 59707409b5a..48a20f498b7 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -30,8 +30,8 @@ describe StuckCiJobsWorker do it "does drop the job and logs the reason" do job.update_columns(yaml_variables: '[{"key" => "value"}]') - expect(Gitlab::Sentry).to receive(:track_acceptable_exception) - .with(anything, a_hash_including(extra: a_hash_including(build_id: job.id))) + expect(Gitlab::Sentry).to receive(:track_exception) + .with(anything, a_hash_including(build_id: job.id)) .once .and_call_original |