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

[CIVIS-2467] Support test visibility protocol via Datadog Agent with EVP proxy #51

Merged
merged 24 commits into from
Oct 24, 2023

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Oct 16, 2023

What does this PR do?
For the case when agentless mode is not configured, we check whether Datadog Agent has EVP (event platform) proxy endpoint and if it does, we use test visibility protocol via this proxy to submit traces to Datadog.

This PR introduces Datadog::CI::Transport::Api module that incapsulates API settings such as host, port, ssl usage, gzip compression, etc.

Datadog::CI::TestVisibility::Transport no longer constructs API layer directly: api gets injected in the transport on Components initialization phase.

Motivation
Implementing the next feature for CI visibility library on a road to GA.

How to test the change?
Run ruby tests locally without DD_CIVISIBLITY_AGENTLESS_ENABLED

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023

Codecov Report

Merging #51 (34958c7) into main (9b83a30) will increase coverage by 0.01%.
Report is 8 commits behind head on main.
The diff coverage is 99.58%.

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   99.15%   99.16%   +0.01%     
==========================================
  Files          95      102       +7     
  Lines        3324     3487     +163     
  Branches      129      134       +5     
==========================================
+ Hits         3296     3458     +162     
- Misses         28       29       +1     
Files Coverage Δ
lib/datadog/ci/configuration/components.rb 100.00% <100.00%> (ø)
lib/datadog/ci/ext/transport.rb 100.00% <100.00%> (ø)
lib/datadog/ci/test_visibility/transport.rb 96.20% <100.00%> (-0.35%) ⬇️
lib/datadog/ci/transport/api/base.rb 100.00% <100.00%> (ø)
lib/datadog/ci/transport/api/builder.rb 100.00% <100.00%> (ø)
lib/datadog/ci/transport/api/ci_test_cycle.rb 100.00% <100.00%> (ø)
lib/datadog/ci/transport/api/evp_proxy.rb 100.00% <100.00%> (ø)
spec/datadog/ci/configuration/components_spec.rb 96.29% <100.00%> (-0.07%) ⬇️
spec/datadog/ci/test_visibility/transport_spec.rb 100.00% <100.00%> (ø)
...pec/datadog/ci/transport/api/ci_test_cycle_spec.rb 100.00% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@anmarchenko anmarchenko force-pushed the anmarchenko/introduce_api_concept branch from 846b448 to 8dbbf5c Compare October 17, 2023 12:45
@anmarchenko anmarchenko changed the title EVP proxy support Support test visibility protocol via Datadog Agent Oct 17, 2023
@anmarchenko anmarchenko changed the title Support test visibility protocol via Datadog Agent [CIVIS-2467] Support test visibility protocol via Datadog Agent Oct 17, 2023
@anmarchenko anmarchenko changed the title [CIVIS-2467] Support test visibility protocol via Datadog Agent [CIVIS-2467] Support test visibility protocol via Datadog Agent with EVP proxy Oct 18, 2023
@anmarchenko anmarchenko requested review from a team and romainkomorndatadog October 18, 2023 14:56
@anmarchenko anmarchenko marked this pull request as ready for review October 18, 2023 14:57
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

The code looks good.

Given this is a sizeable new feature, would it make sense if we had at least one integration test for it? (unless there's already one and I interpreted it as unit test)

@anmarchenko
Copy link
Member Author

@marcotc we run integration tests in https://github.com/DataDog/test-environment but this is a good point, I kind of neglected integration tests that we have in this repo

I will add a task for myself to add more of them

Comment on lines 8 to 9
def request(path:, payload:, verb: "post")
end
Copy link
Member

Choose a reason for hiding this comment

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

We could make the Base class take an Datadog::CI::Transport::HTTP instance in the initialize method + whatever the child needs and move the logic to create the correct HTTP instance to the builder.

Since the request method seems to pass the arguments to the http request method that method is suited to be part of the base class.

And we can make sure that every child class of Base have its own headers method by marking in the base as not implemented

Here is with code what I mean:

# frozen_string_literal: true

module Datadog
  module CI
    module Transport
      module Api
        class Base
          attr_reader :http

          def initialize(http:)
            @http = http
          end

          def request(path:, payload:, verb: "post")
            http.request(
              path: path,
              payload: payload,
              verb: verb,
              headers: headers
            )
          end

          private

          def headers
            raise NotImplementedError
          end
        end
      end
    end
  end
end
# frozen_string_literal: true

require_relative "base"

module Datadog
  module CI
    module Transport
      module Api
        class CIIntake < Base
          attr_reader :api_key

          def initialize(http:, api_key:)
            super(http: http)

            @api_key = api_key
          end

          private

          def headers
            {
              Ext::Transport::HEADER_CONTENT_TYPE => Ext::Transport::CONTENT_TYPE_MESSAGEPACK
              Ext::Transport::HEADER_DD_API_KEY => api_key
            }
          end
        end
      end
    end
  end
end
# frozen_string_literal: true

require_relative "base"

module Datadog
  module CI
    module Transport
      module Api
        class EVPProxy < Base
          def request(path:, payload:, verb: "post")
            path = "#{Ext::Transport::EVP_PROXY_PATH_PREFIX}#{path.sub(/^\//, "")}"

            super(
              path: path,
              payload: payload,
              verb: verb,
              headers: headers
            )
          end

          private

          def container_id
            return @container_id if defined?(@container_id)

            @container_id = Datadog::Core::Environment::Container.container_id
          end

          def headers
            headers = {
              Ext::Transport::HEADER_CONTENT_TYPE => Ext::Transport::CONTENT_TYPE_MESSAGEPACK
              Ext::Transport::HEADER_EVP_SUBDOMAIN => Ext::Transport::TEST_VISIBILITY_INTAKE_HOST_PREFIX
            }

            c_id = container_id
            headers[Ext::Transport::HEADER_CONTAINER_ID] = c_id unless c_id.nil?

            headers
          end
        end
      end
    end
  end
end
# frozen_string_literal: true

require_relative "../http"
require_relative "ci_intake"
require_relative "evp_proxy"

module Datadog
  module CI
    module Transport
      module Api
        module Builder
          def self.build_ci_test_cycle_api(settings)
            dd_site = settings.site || Ext::Transport::DEFAULT_DD_SITE
            url = settings.ci.agentless_url ||
              "https://#{Ext::Transport::TEST_VISIBILITY_INTAKE_HOST_PREFIX}.#{dd_site}:443"

            uri = URI.parse(url)
            raise "Invalid agentless mode URL: #{url}" if uri.host.nil?

            http = Datadog::CI::Transport::HTTP.new(
              host: uri.host,
              port: uri.port,
              ssl: uri.scheme == "https" || uri.port == 443,
              compress: true
            )

            CIIntake.new(http: http, api_key: settings.api_key)
          end

          def self.build_evp_proxy_api(agent_settings)
            http = Datadog::CI::Transport::HTTP.new(
              host: agent_settings.hostname,
              port: agent_settings.port,
              ssl: agent_settings.ssl,
              timeout: agent_settings.timeout_seconds,
              compress: false
            )

            EVPProxy.new(http: http)
          end
        end
      end
    end
  end
end

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, useful suggestion again @GustavoCaso, this can further decouple APIs from underlying transport used, will try to refactor it today

@marcotc
Copy link
Member

marcotc commented Oct 23, 2023

@anmarchenko if this feature is covered by https://github.com/DataDog/test-environment, then all good!

module CI
module Transport
module Api
class CIIntake < Base
Copy link
Member

Choose a reason for hiding this comment

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

CIIntake 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks ugly, I know :( EVPProxy looks not so much better
I think I should drop abbreviations and call them CiIntake and EvpProxy

Copy link
Member Author

Choose a reason for hiding this comment

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

The official name of the intake is CiTestCycle, I will use it

@anmarchenko anmarchenko merged commit ccf7b64 into main Oct 24, 2023
11 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/introduce_api_concept branch October 24, 2023 11:51
@github-actions github-actions bot added this to the 0.3.0 milestone Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants