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

Remove --zipkin-trace-v2 option #10184

Merged
merged 3 commits into from Jun 29, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 0 additions & 7 deletions src/python/pants/bin/local_pants_runner.py
Expand Up @@ -88,15 +88,8 @@ def _init_graph_session(
dynamic_ui = global_scope.dynamic_ui if global_scope.v2 else False
use_colors = global_scope.get("colors", True)

zipkin_trace_v2 = options.for_scope("reporting").zipkin_trace_v2
# TODO(#8658) This should_report_workunits flag must be set to True for
# StreamingWorkunitHandler to receive WorkUnits. It should eventually
# be merged with the zipkin_trace_v2 flag, since they both involve most
# of the same engine functionality, but for now is separate to avoid
# breaking functionality associated with zipkin tracing while iterating on streaming workunit reporting.
stream_workunits = len(options.for_global_scope().streaming_workunits_handlers) != 0
return graph_scheduler_helper.new_session(
zipkin_trace_v2,
RunTracker.global_instance().run_id,
dynamic_ui=dynamic_ui,
use_colors=use_colors,
Expand Down
11 changes: 2 additions & 9 deletions src/python/pants/engine/internals/native.py
Expand Up @@ -174,16 +174,9 @@ def new_execution_request(self) -> PyExecutionRequest:
return PyExecutionRequest()

def new_session(
self,
scheduler,
should_record_zipkin_spans,
dynamic_ui: bool,
build_id,
should_report_workunits: bool,
self, scheduler, dynamic_ui: bool, build_id, should_report_workunits: bool,
) -> PySession:
return PySession(
scheduler, should_record_zipkin_spans, dynamic_ui, build_id, should_report_workunits,
)
return PySession(scheduler, dynamic_ui, build_id, should_report_workunits,)

def new_scheduler(
self,
Expand Down
8 changes: 2 additions & 6 deletions src/python/pants/engine/internals/scheduler.py
Expand Up @@ -350,17 +350,13 @@ def garbage_collect_store(self):
self._native.lib.garbage_collect_store(self._scheduler)

def new_session(
self,
zipkin_trace_v2: bool,
build_id,
dynamic_ui: bool = False,
should_report_workunits: bool = False,
self, build_id, dynamic_ui: bool = False, should_report_workunits: bool = False,
) -> "SchedulerSession":
"""Creates a new SchedulerSession for this Scheduler."""
return SchedulerSession(
self,
self._native.new_session(
self._scheduler, zipkin_trace_v2, dynamic_ui, build_id, should_report_workunits,
self._scheduler, dynamic_ui, build_id, should_report_workunits,
),
)

Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/engine/internals/scheduler_test_base.py
Expand Up @@ -76,9 +76,7 @@ def mk_scheduler(
include_trace_on_error=include_trace_on_error,
)
return scheduler.new_session(
zipkin_trace_v2=False,
build_id="buildid_for_test",
should_report_workunits=should_report_workunits,
build_id="buildid_for_test", should_report_workunits=should_report_workunits,
)

def context_with_scheduler(self, scheduler, *args, **kwargs):
Expand Down
11 changes: 2 additions & 9 deletions src/python/pants/init/engine_initializer.py
Expand Up @@ -174,16 +174,9 @@ class LegacyGraphScheduler:
goal_map: Any

def new_session(
self,
zipkin_trace_v2,
build_id,
dynamic_ui: bool = False,
use_colors=True,
should_report_workunits=False,
self, build_id, dynamic_ui: bool = False, use_colors=True, should_report_workunits=False,
) -> "LegacyGraphSession":
session = self.scheduler.new_session(
zipkin_trace_v2, build_id, dynamic_ui, should_report_workunits
)
session = self.scheduler.new_session(build_id, dynamic_ui, should_report_workunits)
console = Console(use_colors=use_colors, session=session if dynamic_ui else None,)
return LegacyGraphSession(session, console, self.build_file_aliases, self.goal_map)

Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/pantsd/service/scheduler_service.py
Expand Up @@ -49,9 +49,7 @@ def __init__(
self._scheduler = legacy_graph_scheduler.scheduler
# This session is only used for checking whether any invalidation globs have been invalidated.
# It is not involved with a build itself; just with deciding when we should restart pantsd.
self._scheduler_session = self._scheduler.new_session(
zipkin_trace_v2=False, build_id="scheduler_service_session",
)
self._scheduler_session = self._scheduler.new_session(build_id="scheduler_service_session",)
self._logger = logging.getLogger(__name__)

# NB: We declare these as a single field so that they can be changed atomically.
Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/pantsd/service/store_gc_service.py
Expand Up @@ -23,9 +23,7 @@ def __init__(
gc_interval_secs=(4 * 60 * 60),
):
super().__init__()
self._scheduler_session = scheduler.new_session(
zipkin_trace_v2=False, build_id="store_gc_service_session"
)
self._scheduler_session = scheduler.new_session(build_id="store_gc_service_session")
self._logger = logging.getLogger(__name__)

self._period_secs = period_secs
Expand Down
7 changes: 0 additions & 7 deletions src/python/pants/reporting/reporting.py
Expand Up @@ -94,13 +94,6 @@ def register_options(cls, register):
default=100.0,
help="Rate at which to sample Zipkin traces. Value 0.0 - 100.0.",
)
Copy link
Sponsor Member

@stuhood stuhood Jun 27, 2020

Choose a reason for hiding this comment

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

I expect that there is a lot more code in here related to zipkin that can be removed.

register(
"--zipkin-trace-v2",
advanced=True,
type=bool,
default=False,
help="If enabled, the zipkin spans are tracked for v2 engine execution progress.",
)
register(
"--zipkin-service-name-prefix",
advanced=True,
Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/testutil/test_base.py
Expand Up @@ -429,9 +429,7 @@ def _init_engine(self, local_store_dir: Optional[str] = None) -> None:
build_configuration=self.build_config(),
build_ignore_patterns=None,
execution_options=ExecutionOptions.from_bootstrap_options(global_options),
).new_session(
zipkin_trace_v2=False, build_id="buildid_for_test", should_report_workunits=True
)
).new_session(build_id="buildid_for_test", should_report_workunits=True)
self._scheduler = graph_session.scheduler_session
self._build_graph, self._address_mapper = graph_session.create_build_graph(
Specs(address_specs=AddressSpecs([]), filesystem_specs=FilesystemSpecs([])),
Expand Down
10 changes: 1 addition & 9 deletions src/rust/engine/src/externs/interface.rs
Expand Up @@ -498,14 +498,12 @@ py_class!(class PySession |py| {
data session: Session;
def __new__(_cls,
scheduler_ptr: PyScheduler,
should_record_zipkin_spans: bool,
should_render_ui: bool,
build_id: String,
should_report_workunits: bool
) -> CPyResult<Self> {
Self::create_instance(py, Session::new(
scheduler_ptr.scheduler(py),
should_record_zipkin_spans,
should_render_ui,
build_id,
should_report_workunits,
Expand Down Expand Up @@ -927,17 +925,11 @@ fn scheduler_metrics(
) -> CPyResult<PyObject> {
with_scheduler(py, scheduler_ptr, |scheduler| {
with_session(py, session_ptr, |session| {
let mut values = scheduler
let values = scheduler
.metrics(session)
.into_iter()
.map(|(metric, value)| (externs::store_utf8(metric), externs::store_i64(value)))
.collect::<Vec<_>>();
if session.should_record_zipkin_spans() {
let workunits = session.workunit_store().get_workunits();
let mut iter = workunits.iter();
let value = workunits_to_py_tuple_value(&mut iter, &scheduler.core)?;
values.push((externs::store_utf8("engine_workunits"), value));
};
externs::store_dict(values).map(|d| d.consume_into_py_object(py))
})
})
Expand Down
12 changes: 0 additions & 12 deletions src/rust/engine/src/scheduler.rs
Expand Up @@ -58,8 +58,6 @@ struct InnerSession {
// If enabled, the display that will render the progress of the V2 engine. This is only
// Some(_) if the --dynamic-ui option is enabled.
display: Option<Mutex<ConsoleUI>>,
// If enabled, Zipkin spans for v2 engine will be collected.
should_record_zipkin_spans: bool,
// A place to store info about workunits in rust part
workunit_store: WorkunitStore,
// The unique id for this Session: used for metrics gathering purposes.
Expand All @@ -81,7 +79,6 @@ pub struct Session(Arc<InnerSession>);
impl Session {
pub fn new(
scheduler: &Scheduler,
should_record_zipkin_spans: bool,
should_render_ui: bool,
build_id: String,
should_report_workunits: bool,
Expand All @@ -97,7 +94,6 @@ impl Session {
preceding_graph_size: scheduler.core.graph.len(),
roots: Mutex::new(HashMap::new()),
display,
should_record_zipkin_spans,
workunit_store,
build_id,
run_id: Mutex::new(Uuid::new_v4()),
Expand Down Expand Up @@ -131,10 +127,6 @@ impl Session {
self.0.preceding_graph_size
}

pub fn should_record_zipkin_spans(&self) -> bool {
self.0.should_record_zipkin_spans
}

pub fn should_report_workunits(&self) -> bool {
self.0.should_report_workunits
}
Expand Down Expand Up @@ -185,10 +177,6 @@ impl Session {
}
}

pub fn should_handle_workunits(&self) -> bool {
self.should_report_workunits() || self.should_record_zipkin_spans()
}

fn maybe_display_initialize(&self, executor: &Executor, sender: &mpsc::Sender<ExecutionEvent>) {
if let Some(display) = &self.0.display {
let mut display = display.lock();
Expand Down
15 changes: 0 additions & 15 deletions src/rust/engine/workunit_store/src/lib.rs
Expand Up @@ -201,21 +201,6 @@ impl WorkunitStore {
}))
}

pub fn get_workunits(&self) -> Vec<Workunit> {
let mut inner_guard = (*self.inner).lock();
let inner_store: &mut WorkUnitInnerStore = &mut *inner_guard;
let workunit_records = &inner_store.workunit_records;
inner_store
.completed_ids
.iter()
.flat_map(|id| workunit_records.get(id))
.flat_map(|workunit| match workunit.state {
WorkunitState::Started { .. } => None,
WorkunitState::Completed { .. } => Some(workunit.clone()),
})
.collect()
}

///
/// Find the longest running leaf workunits, and render their first visible parents.
///
Expand Down
Expand Up @@ -22,7 +22,6 @@ def test_stats_local_json_file_v2(self):
"test",
f"--run-tracker-stats-local-json-file={tmpfile}",
"--run-tracker-stats-version=2",
"--reporting-zipkin-trace-v2",
"testprojects/src/java/org/pantsbuild/testproject/unicode/main",
]
)
Expand Down
63 changes: 0 additions & 63 deletions tests/python/pants_test/reporting/test_reporting_integration.py
Expand Up @@ -9,7 +9,6 @@
import psutil

from pants.testutil.pants_run_integration_test import PantsRunIntegrationTest
from pants.util.collections import assert_single_element
from pants.util.contextutil import http_server


Expand Down Expand Up @@ -51,68 +50,6 @@ def test_zipkin_reporter_with_zero_sample_rate(self):
num_of_traces = len(ZipkinHandler.traces)
self.assertEqual(num_of_traces, 0)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The test above this looks dead as well?

def test_zipkin_reporter_for_v2_engine(self):
ZipkinHandler = zipkin_handler()
with http_server(ZipkinHandler) as port:
endpoint = f"http://localhost:{port}"
command = [
"-ldebug",
f"--reporting-zipkin-endpoint={endpoint}",
"--reporting-zipkin-trace-v2",
"list",
"examples/src/python/example/hello/greet",
]

pants_run = self.run_pants(command)
self.assert_success(pants_run)

child_processes = self.find_child_processes_that_send_spans(pants_run.stderr_data)
self.assertTrue(child_processes)

self.wait_spans_to_be_sent(child_processes)

trace = assert_single_element(ZipkinHandler.traces.values())

v2_span_name_part = "snapshot"
self.assertTrue(
any(v2_span_name_part in span["name"] for span in trace),
"There is no span that contains '{}' in it's name. The trace:{}".format(
v2_span_name_part, trace
),
)

def test_zipkin_reports_for_pure_v2_goals(self):
ZipkinHandler = zipkin_handler()
with http_server(ZipkinHandler) as port:
endpoint = f"http://localhost:{port}"
command = [
"-ldebug",
"--no-v1",
"--v2",
f"--reporting-zipkin-endpoint={endpoint}",
"--reporting-zipkin-trace-v2",
"list",
"3rdparty:",
]

pants_run = self.run_pants(command)
self.assert_success(pants_run)

child_processes = self.find_child_processes_that_send_spans(pants_run.stderr_data)
self.assertTrue(child_processes)

self.wait_spans_to_be_sent(child_processes)

trace = assert_single_element(ZipkinHandler.traces.values())

v2_span_name_part = "snapshot"
self.assertTrue(
any(v2_span_name_part in span["name"] for span in trace),
"There is no span that contains '{}' in it's name. The trace:{}".format(
v2_span_name_part, trace
),
)

@staticmethod
def find_spans_by_name_and_service_name(trace, name, service_name):
return [
Expand Down