Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDTEST-160] git commands telemetry #205

Merged
merged 5 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 116 additions & 25 deletions lib/datadog/ci/git/local_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,25 @@
require "open3"
require "pathname"

require_relative "../ext/telemetry"
require_relative "telemetry"
require_relative "user"

module Datadog
module CI
module Git
module LocalRepository
class GitCommandExecutionError < StandardError
attr_reader :output, :command, :status
def initialize(message, output:, command:, status:)
super(message)

@output = output
@command = command
@status = status
end
end

COMMAND_RETRY_COUNT = 3

def self.root
Expand Down Expand Up @@ -48,9 +61,18 @@ def self.current_folder_name
end

def self.git_repository_url
exec_git_command("git ls-remote --get-url")
Telemetry.git_command(Ext::Telemetry::Command::GET_REPOSITORY)
res = nil

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
res = exec_git_command("git ls-remote --get-url")

Choose a reason for hiding this comment

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

Small question: why not have exec_git_command() do the measuring and return some kind of tuple with result, duration, and status code? Or alternatively, you could pass the git command as a parameter and have the underlying method do the telemetry piece.

It would be a very modest win in code deduplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of DRY in general and I eager to accept code duplication every time over confusing behaviour.

Changing return of the method to include telemetry mixes 2 different concepts together: executing command and measuring it. If I return back to this code after 2 months, I will never remember why exec_git_command returns a tuple. What's worse, we measure only subset of git commands (for whatever reason). This means that these return values will be used in some cases but ignored in other and it will be absolutely impossible to explain why.

In this case deduplicating the code would completely break exec_git_command abstraction: I will take duplication over broken abstraction every day (like Sandi taught me! :) )

Choose a reason for hiding this comment

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

I ended up inserting a wrapped function for the Python tracer... but since I had to maintain backwards compatibility, I kept the original function as a wrapper of the new one, which may or may not have been the right move.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with duplication for now, but I will refactor if it'll become painful in the future

end

Telemetry.git_command_ms(Ext::Telemetry::Command::GET_REPOSITORY, duration_ms)
res
rescue => e
log_failure(e, "git repository url")
telemetry_track_error(e, Ext::Telemetry::Command::GET_REPOSITORY)
nil
end

Expand All @@ -69,9 +91,18 @@ def self.git_commit_sha
end

def self.git_branch
exec_git_command("git rev-parse --abbrev-ref HEAD")
Telemetry.git_command(Ext::Telemetry::Command::GET_BRANCH)
res = nil

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
res = exec_git_command("git rev-parse --abbrev-ref HEAD")
end

Telemetry.git_command_ms(Ext::Telemetry::Command::GET_BRANCH, duration_ms)
res
rescue => e
log_failure(e, "git branch")
telemetry_track_error(e, Ext::Telemetry::Command::GET_BRANCH)
nil
end

Expand Down Expand Up @@ -116,29 +147,49 @@ def self.git_commit_users

# returns maximum of 1000 latest commits in the last month
def self.git_commits
output = exec_git_command("git log --format=%H -n 1000 --since=\"1 month ago\"")
Telemetry.git_command(Ext::Telemetry::Command::GET_LOCAL_COMMITS)

output = nil
duration_ms = Core::Utils::Time.measure(:float_millisecond) do
output = exec_git_command("git log --format=%H -n 1000 --since=\"1 month ago\"")
end

Telemetry.git_command_ms(Ext::Telemetry::Command::GET_LOCAL_COMMITS, duration_ms)

return [] if output.nil?

# @type var output: String
output.split("\n")
rescue => e
log_failure(e, "git commits")
telemetry_track_error(e, Ext::Telemetry::Command::GET_LOCAL_COMMITS)
[]
end

def self.git_commits_rev_list(included_commits:, excluded_commits:)
Telemetry.git_command(Ext::Telemetry::Command::GET_OBJECTS)
included_commits = filter_invalid_commits(included_commits).join(" ")
excluded_commits = filter_invalid_commits(excluded_commits).map! { |sha| "^#{sha}" }.join(" ")

exec_git_command(
"git rev-list " \
"--objects " \
"--no-object-names " \
"--filter=blob:none " \
"--since=\"1 month ago\" " \
"#{excluded_commits} #{included_commits}"
)
res = nil

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
res = exec_git_command(
"git rev-list " \
"--objects " \
"--no-object-names " \
"--filter=blob:none " \
"--since=\"1 month ago\" " \
"#{excluded_commits} #{included_commits}"
)
end

Telemetry.git_command_ms(Ext::Telemetry::Command::GET_OBJECTS, duration_ms)

res
rescue => e
log_failure(e, "git commits rev list")
telemetry_track_error(e, Ext::Telemetry::Command::GET_OBJECTS)
nil
end

Expand All @@ -150,35 +201,59 @@ def self.git_generate_packfiles(included_commits:, excluded_commits:, path:)

basename = SecureRandom.hex(4)

exec_git_command(
"git pack-objects --compression=9 --max-pack-size=3m #{path}/#{basename}",
stdin: commit_tree
)
Telemetry.git_command(Ext::Telemetry::Command::PACK_OBJECTS)

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
exec_git_command(
"git pack-objects --compression=9 --max-pack-size=3m #{path}/#{basename}",
stdin: commit_tree
)
end
Telemetry.git_command_ms(Ext::Telemetry::Command::PACK_OBJECTS, duration_ms)

basename
rescue => e
log_failure(e, "git generate packfiles")
telemetry_track_error(e, Ext::Telemetry::Command::PACK_OBJECTS)
nil
end

def self.git_shallow_clone?
exec_git_command("git rev-parse --is-shallow-repository") == "true"
Telemetry.git_command(Ext::Telemetry::Command::CHECK_SHALLOW)
res = false

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
res = exec_git_command("git rev-parse --is-shallow-repository") == "true"
end
Telemetry.git_command_ms(Ext::Telemetry::Command::CHECK_SHALLOW, duration_ms)

res
rescue => e
log_failure(e, "git shallow clone")
telemetry_track_error(e, Ext::Telemetry::Command::CHECK_SHALLOW)
false
end

def self.git_unshallow
exec_git_command(
"git fetch " \
"--shallow-since=\"1 month ago\" " \
"--update-shallow " \
"--filter=\"blob:none\" " \
"--recurse-submodules=no " \
"$(git config --default origin --get clone.defaultRemoteName) $(git rev-parse HEAD)"
)
Telemetry.git_command(Ext::Telemetry::Command::UNSHALLOW)
res = nil

duration_ms = Core::Utils::Time.measure(:float_millisecond) do
res = exec_git_command(
"git fetch " \
"--shallow-since=\"1 month ago\" " \
"--update-shallow " \
"--filter=\"blob:none\" " \
"--recurse-submodules=no " \
"$(git config --default origin --get clone.defaultRemoteName) $(git rev-parse HEAD)"
)
end
Telemetry.git_command_ms(Ext::Telemetry::Command::UNSHALLOW, duration_ms)

res
rescue => e
log_failure(e, "git unshallow")
telemetry_track_error(e, Ext::Telemetry::Command::UNSHALLOW)
nil
end

Expand Down Expand Up @@ -209,7 +284,12 @@ def exec_git_command(cmd, stdin: nil)
end

if status.nil? || !status.success?
raise "Failed to run git command [#{cmd}] with input [#{stdin}] and output [#{out}]"
raise GitCommandExecutionError.new(
"Failed to run git command [#{cmd}] with input [#{stdin}] and output [#{out}]",
output: out,
command: cmd,
status: status
)
end

# Sometimes Encoding.default_external is somehow set to US-ASCII which breaks
Expand All @@ -231,6 +311,17 @@ def log_failure(e, action)
"Unable to perform #{action}: #{e.class.name} #{e.message} at #{Array(e.backtrace).first}"
)
end

def telemetry_track_error(e, command)
case e
when Errno::ENOENT
Telemetry.git_command_errors(command, executable_missing: true)
when GitCommandExecutionError
Telemetry.git_command_errors(command, exit_code: e.status&.to_i)
else
Telemetry.git_command_errors(command, exit_code: -9000)
end
end
end
end
end
Expand Down
37 changes: 37 additions & 0 deletions lib/datadog/ci/git/telemetry.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# frozen_string_literal: true

module Datadog
module CI
module Git
module Telemetry
def self.git_command(command)
Utils::Telemetry.inc(Ext::Telemetry::METRIC_GIT_COMMAND, 1, tags_for_command(command))
end

def self.git_command_errors(command, exit_code: nil, executable_missing: false)
tags = tags_for_command(command)

exit_code_tag_value = exit_code_for(exit_code: exit_code, executable_missing: executable_missing)
tags[Ext::Telemetry::TAG_EXIT_CODE] = exit_code_tag_value if exit_code_tag_value

Utils::Telemetry.inc(Ext::Telemetry::METRIC_GIT_COMMAND_ERRORS, 1, tags)
end

def self.git_command_ms(command, duration_ms)
Utils::Telemetry.distribution(Ext::Telemetry::METRIC_GIT_COMMAND_MS, duration_ms, tags_for_command(command))
end

def self.tags_for_command(command)
{Ext::Telemetry::TAG_COMMAND => command}
end

def self.exit_code_for(exit_code: nil, executable_missing: false)
return Ext::Telemetry::ExitCode::MISSING if executable_missing
return exit_code.to_s if exit_code

nil
end
end
end
end
end
3 changes: 1 addition & 2 deletions lib/datadog/ci/test_visibility/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
require_relative "../codeowners/parser"
require_relative "../contrib/contrib"
require_relative "../ext/test"
require_relative "../ext/environment"
require_relative "../git/local_repository"

require_relative "../worker"
Expand All @@ -28,7 +27,7 @@ def initialize(
codeowners: Codeowners::Parser.new(Git::LocalRepository.root).parse
)
@test_suite_level_visibility_enabled = test_suite_level_visibility_enabled
@context = Context.new(Ext::Environment.tags(ENV).freeze)
@context = Context.new
@codeowners = codeowners
@test_optimisation = test_optimisation
@remote_settings_api = remote_settings_api
Expand Down
7 changes: 4 additions & 3 deletions lib/datadog/ci/test_visibility/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require_relative "store/local"

require_relative "../ext/app_types"
require_relative "../ext/environment"
require_relative "../ext/test"

require_relative "../span"
Expand All @@ -23,11 +24,9 @@ module TestVisibility
# Its responsibility includes building domain models for test visibility as well.
# Internally it uses Datadog::Tracing module to create spans.
class Context
def initialize(environment_tags)
def initialize
@local_context = Store::Local.new
@global_context = Store::Global.new

@environment_tags = environment_tags
end

def start_test_session(service: nil, tags: {})
Expand Down Expand Up @@ -197,6 +196,8 @@ def build_span(tracer_span, tags)

# TAGGING
def set_initial_tags(ci_span, tags)
@environment_tags ||= Ext::Environment.tags(ENV).freeze

ci_span.set_default_tags
ci_span.set_environment_runtime_tags

Expand Down
10 changes: 10 additions & 0 deletions sig/datadog/ci/git/local_repository.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@ module Datadog
module CI
module Git
module LocalRepository
class GitCommandExecutionError < StandardError
attr_reader command: String
attr_reader output: String?
attr_reader status: Process::Status?

def initialize: (String message, output: String?, status: Process::Status?, command: String) -> void
end

COMMAND_RETRY_COUNT: 3

@root: String?
Expand Down Expand Up @@ -46,6 +54,8 @@ module Datadog
def self.exec_git_command: (String ref, ?stdin: String?) -> String?

def self.log_failure: (StandardError e, String action) -> void

def self.telemetry_track_error: (StandardError e, String command) -> void
end
end
end
Expand Down
17 changes: 17 additions & 0 deletions sig/datadog/ci/git/telemetry.rbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module Datadog
module CI
module Git
module Telemetry
def self.git_command: (String command) -> void

def self.git_command_errors: (String command, ?exit_code: Integer?, ?executable_missing: bool) -> void

def self.git_command_ms: (String command, untyped duration_ms) -> void

def self.tags_for_command: (String command) -> ::Hash[String, String]

def self.exit_code_for: (?exit_code: Integer?, ?executable_missing: bool) -> String?
end
end
end
end
2 changes: 1 addition & 1 deletion sig/datadog/ci/test_visibility/context.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module Datadog

@environment_tags: Hash[String, String]

def initialize: (Hash[String, String] environment_tags) -> void
def initialize: () -> void

def trace_test: (String span_name, String test_suite_name, ?service: String?, ?tags: Hash[untyped, untyped]) ?{ (Datadog::CI::Test span) -> untyped } -> untyped

Expand Down
8 changes: 0 additions & 8 deletions spec/datadog/ci/configuration/components_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@
context "is enabled" do
let(:enabled) { true }

it "collects environment tags" do
expect(Datadog::CI::Ext::Environment).to have_received(:tags).with(ENV)
end

context "when tracing is disabled" do
let(:tracing_enabled) { false }

Expand Down Expand Up @@ -345,10 +341,6 @@
expect(settings.tracing.test_mode)
.to_not have_received(:writer_options=)
end

it "does not collect tags" do
expect(Datadog::CI::Ext::Environment).not_to have_received(:tags)
end
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@
steps_file_for_run_path
)

expect(Datadog::CI::Ext::Environment).to receive(:tags).never
# assert that environment tags are collected once per session
expect(Datadog::CI::Ext::Environment).to receive(:tags).once.and_call_original

expect(kernel).to receive(:exit).with(expected_test_run_code)

# do not use manual API
Expand Down
Loading
Loading