Fix runtime workers not flushing to Dogstatsd #939
Conversation
majorgreys
commented
May 15, 2019
•
majorgreys
commented
- Fixes shutdown of runtime workers and add test
- Avoid unnecessary setting services
| @@ -330,6 +334,10 @@ def _check_new_process(self): | |||
|
|
|||
| self._start_runtime_worker() | |||
|
|
|||
| # force an immediate update constant tags since we have reset services | |||
| # and generated a new runtime id | |||
| self._update_dogstatsd_constant_tags() | |||
There was a problem hiding this comment.
This will run for every single trace being finished, is that what we want?
There was a problem hiding this comment.
This is the major change for this PR right? Everything else looks like some organization work?
There was a problem hiding this comment.
This would be run whenever a new child process is forked. And, right, the other changes could be removed to keep this PR focused.
| # update set of services handled by tracer | ||
| if service: | ||
| if service and service not in self._services: |
There was a problem hiding this comment.
Noticed by looking at logs that we were updating self._services and constant tags with each span, which is definitely overkill. So added this membership test before adding a service.
majorgreys
force-pushed
the
majorgreys/runtime-tag-fix
branch
2 times, most recently
from
e898373 to
36161f3
Compare
majorgreys
force-pushed
the
majorgreys/runtime-tag-fix
branch
from
36161f3 to
144d10d
Compare
| @@ -60,7 +60,8 @@ class RuntimeWorker(_worker.PeriodicWorkerThread): | |||
|
|
|||
| FLUSH_INTERVAL = 10 | |||
|
|
|||
| def __init__(self, statsd_client, flush_interval=FLUSH_INTERVAL): | |||
| def __init__(self, statsd_client, flush_interval=None): | |||
| flush_interval = self.FLUSH_INTERVAL if flush_interval is None else flush_interval | |||
There was a problem hiding this comment.
We did this because we had trouble finding a way to override the flush interval in a test case.
This isn't ideal and should likely be refactored into something else.
Maybe:
class Tracer:
def configure(self, ... _runtime_flush_interval=10):
pass
This is what we are doing that required us to make this change:
dd-trace-py/tests/internal/runtime/test_runtime_metrics.py
Lines 53 to 62 in 144d10d
There was a problem hiding this comment.
Fair enough.
Yeah configure should allow to customize that ultimately.
There was a problem hiding this comment.
yeah, I think the main think we wanted to test was end-to-end letting the tracer configure the runtime worker, did the right thing
| @@ -78,7 +79,7 @@ def flush(self): | |||
| for key, value in self._runtime_metrics: | |||
| self._write_metric(key, value) | |||
|
|
|||
| on_periodic = flush | |||
| run_periodic = flush | |||
There was a problem hiding this comment.
🤦♂
I guess the tests passed because we call flush manually?
There was a problem hiding this comment.
yeah, we call flush on stop()
majorgreys
changed the title
