-
Notifications
You must be signed in to change notification settings - Fork 4
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
[CIAPP-2959] CI visibility protocol and agentless mode support #33
Conversation
Codecov Report
@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 98.94% 99.18% +0.23%
==========================================
Files 82 95 +13
Lines 2563 3437 +874
Branches 98 125 +27
==========================================
+ Hits 2536 3409 +873
- Misses 27 28 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
serialize test event to msgpack
remove payload logs as payload is binary
There's a lot to digest here. Many of the changes you've been making are great! (e.g. I like driving the separation between CI and Tracing contrib). I do have one area of concern though:
This leverages the old transport. In the future, tracing will NOT support this as a drop-in replacement (likely in 2.0.) Is your plan to re-implement CI's transport on the new trace transport when that happens? I think this speaks to my concern that CI should not be replacing Tracing internals: you might destabilize Tracing. We do not give any public API guarantees around the use of Transport code within the |
@delner thanks a lot for the additional context! I explained more of my motivation to go this way in my PR to ddtrace that was accepted last week: DataDog/dd-trace-rb#3158
I think adapting CI transport would be still less work than copypasting and maintaining TraceWriter and TraceBuffer in CI gem, so this would be my plan, yes
I would happily help us to define a new concept of tracing's extensibility then! I am very optimistic about our prospects to find a solution to make Tracer to submit traces to CI endpoint with CI serializers.
could you explain more? I am clearly missing some context here: CI visibility will never run in production and will be included by CI customers as a separate library, so right now I don't see how could we destabilize tracing. If the concern here is that providing configurable transport will open path for external developers to replace this part of tracing without understanding the underlying complexity then removing configuration will help not that much: it is still possible to replace tracer's logic with monkey patching.
if writer options will be removed in the future and we won't create any replacement then I have a backup plan to implement TraceWriter and Tracing::Buffer in the CI gem. It will essentially duplicate the logic that already exists in Tracing module. Then I will use CI::Flush to consume traces and send them to CI::TraceWriter that will store them in CI::TraceBuffer and send via CI::Transport. I would not want to go this way now because it means to copypaste the code that already exists in Tracing to this library and maintaining it: currently it would not help us to achieve our goal of releasing this library to GA. This is still an option for the future though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 nice! Left some minor comments but LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anmarchenko and I discussed the "Datadog::CI::TestVisibility::Transport is a drop-in replacement for tracer's transport" change.
- I shared why the current behavior is undesirable (as it touches a lot of internals of how Tracing works)
- I suggested an alternative strategy where
CI
can subscribe totrace_completed
, then reuse theCore::Workers
andCore::Buffer
components to build its own worker to ingest traces, transform them toCI
events, and submit them to the API. @anmarchenko and I agreed this would work forCI
's needs. - @anmarchenko and I agreed to implement the above strategy in 2.x:
Tracing
will provide an API so thatCI
can subscribe & receive completed traces, thenTracing
will remove support for modifying itsWriter
.CI
will re-implement this feature on this new public API. - In the interim, in 1.x,
Tracing
will continue to support the existingtransport_options
andwriter
configurability on which this PR is built.
Therefore, there's no blocker for merging this today.
What does this PR do?
Decouples Contrib module and configuration from ddtrace
Adds agentless mode settings
Adds HTTP layer with gzip support
Adds TestVisibility transport and configures ddtrace Tracer to use it
send_traces
method that receives a list of traces, serializes them to msgpack, chunks by size (at most 4Mb per request) and sends to CI visibility intakeAdds a layer of TestVisbility serializers and factories
content
tag, required fields and some field level logicMotivation
Implementing long-awaited agentless mode for Ruby test visibility.
How to test the change?
Used https://github.com/anmarchenko/sidekiq for integration testing.
git clone
this repo and thenDD_API_KEY=<your key> bundle exec rake