Add CI mode #1504
Add CI mode #1504
Conversation
|
Might also want to add documentation for the |
Codecov Report
@@ Coverage Diff @@
## master #1504 +/- ##
==========================================
- Coverage 98.20% 98.19% -0.01%
==========================================
Files 863 867 +4
Lines 41779 41883 +104
==========================================
+ Hits 41028 41129 +101
- Misses 751 754 +3
Continue to review full report at Codecov.
|
|
|
||
| # Temporary adjustment for flushing test spans: | ||
| # Flushes spans more aggressive from the buffer. | ||
| # Replace this with span streaming or synchronous writing. |
There was a problem hiding this comment.
In these test environments, is the trace-agent running in syncMode as well, ala how it runs in Lambda? I do think that sync writing makes sense to work toward if we need high guarantees that all spans flush.
There was a problem hiding this comment.
Right now, no. But it could, potentially.
Downside with synchronous writing is that it doesn't play nicely with some anticipated use.
I would like users to be able to assert things about traces produced within their tests (e.g. "I expected this web request to produce a trace with a Rack span") This would be especially useful for users who have defined manual instrumentation and want to verify its correctness.
If users use the RSpec or Cucumber integration to trace their CI tests, the root span of their traces will be the span for the test (e.g. RSpec span.) Because they would be running assertions on trace output while the test is still running, the trace would not be complete and not written to the writer's trace buffer. Any assertions on trace output will fail, because the trace will not be in the buffer to assert against.
This is where partial flush of size 1 comes in: it would "stream" each span from the context immediately to the writer's buffer, meaning you could assert on written spans. It also has the bonus of tagging every span with "origin", something we currently require of all spans generated during a CI test.
Sync writing doesn't play well with this, because if we activated it with partial flushing size 1, it would do a blocking HTTP request for every span in the test... this would be very bad.
There may be a better strategy to get these two to play together, but I don't have one at the moment. If you have suggestions I'd be happy to hear them.
There was a problem hiding this comment.
Yes i think sync mode would mean moving away from how we currently do partial flushing, we would instead be queuing many traces/spans in memory (perhaps >10k), and then doing a few large flushes. I would have to review how the agent handles syncronous flushing, ideally we could model our approach after theirs.
There was a problem hiding this comment.
(this can be resolved if there are no other thoughts here, i dont think this applies to this pr)
There was a problem hiding this comment.
I think this might be worth engaging with more, now, if there's some reasonable, effective change we can make. I'd rather not mess with buffer/interval settings and expose a public API for it if it isn't comprehensive and there's a better alternative readily available (something involving sync writer.)
I will consider alternate strategies, and share my findings.
There was a problem hiding this comment.
I decided to go a different direction, based on the feedback.
By default, CI will turn on a SyncWriter & a custom context flush (same as the default, but adds tags.) Partial flushing no longer applies. This should help improve the guarantee that all spans are flushed without having conflicts with partial flush.
Let me know what you think.
There was a problem hiding this comment.
I think this makes sense. Am I right in saying that the context_flush bit about adding tags is not yet implemented?
I think a combination of context_flush + SyncWriter are correct here. The test_mode i'd be curious to see whether users find it to be useful, i know there have been a lot of requests for it, so that's a nice bonus we can get out of this work.
There was a problem hiding this comment.
I think this comment thread can be resolved when you feel appropriate
There was a problem hiding this comment.
Left a few comments and questions that are not blocking. It was a bit hard to tell what's new code bc of the CI refactor, but I generally think this approach with forcing "span streaming" is a nifty way accomplish adding origin to every span. Not sure about the practicality of this approach outside of a test environment, but as users won't be enabling outside of that environment, then it's fine with me. Working to a sync writer is a good goal.
I also think this work is a good indication that we'll need to anticipate future use cases of trace or transaction level metadata being attached to every exported span. Although, again, I understand the rfc/practicality limits what we can do.
looks like a few tasks open on PR but I think this is good to approve otherwise
|
@ericmustin Sounds like we're pretty much on the same page. If it helps to review the code, you could look at the commits instead of the full list of changes: the commits after the refactor are pretty small. |
|
Updated my TODO list per review feedback; see original comment. |
delner
force-pushed
the
feature/test_mode
branch
2 times, most recently
from
37fd26a
to
a6e6c8d
Compare
|
@ericmustin I've added a spec to RSpec and Cucumber that activates this mode and asserts the presence of this tag on every span. That should be the test that we need. @ericmustin @marcotc should be ready for a final review (might need to get CI passing though, too.) |
There was a problem hiding this comment.
I haven't looked too closely at the 3.0 failures, but setting those aside, some small Qs but i think this lgtm. awesome work.
Not blocking, but i do want to re-iterate that we may want to think about how to extend ContextFlush::Tagging.get_trace in the future. Currently we add origin as a tag, but I envision a scenario in the future where we will need additional, arbitrary, vendor specific tags appended to every span at export/encode time (trace or transaction -level tags). we may want to think about origin as simply a particular tag name within a more inclusive context.trace_state field, which is a w3c concept https://w3c.github.io/trace-context/#tracestate-header. Instead of attach_origin we could simply expose a method like attach_trace_state(span, trace_state_tag_name) or attach_all_trace_state(span).
Don't want to over-engineer for a future hypothetical, but i anticipate requests like this becoming more regular, so just offering this for posterity/future reference if this becomes a pain point
|
@ericmustin I really like the idea for "attach trace state"; my recommendation is the next time we have to decorate more state that we seriously considering implementing this pattern. May be premature for the first pass, but justified for the second. |
|
As a reminder to ourselves, this is how one configures CI tracing: # spec_helper.rb
require 'datadog/ci'# Enable CI tracing
Datadog.configure do |c|
c.ci_mode.enabled = true
c.use :rspec
end |
This pull request improves support for CI tracing by adding a "CI" mode to the tracer, which can be activated to configure settings more suitable for a test suite that runs in CI.
Users can activate CI mode via the following:
Description
Test mode:
The tracer-level implementation, who's purpose is to configure tracing to be test friendly (capture all spans, permit diversion to buffer or alternate output.) It is not CI aware; it merely facilitates reconfiguration.
c.test_mode.enabled/DD_TRACE_TEST_MODE_ENABLEDto enableAllSampler(100% sampler)SyncWriter(to ensure all spans are captured)context_flushorwriter_optionsif desiredCI mode:
The CI-level implementation, who's purpose is to configure a tracing system to produce traces suitable for CI ingestion. It is a superset configuration to
test_mode.c.ci_mode.enabled/DD_TRACE_CI_MODE_ENABLEDto enabletest_modeCI::ContextFlush::Finished)Work in progress:
Tests: