adding dynamic setting xpack.apm.tracing.enabled#80796
adding dynamic setting xpack.apm.tracing.enabled#80796idegtiarenko merged 10 commits intoelastic:feature/apm-integrationfrom
xpack.apm.tracing.enabled#80796Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
| } | ||
|
|
||
| private void setEnabled(boolean enabled) { | ||
| this.enabled = enabled; |
There was a problem hiding this comment.
Could we completely shut everything down if this is set to false? Otherwise we have a bunch of threads and other things running in the background, which shouldn't really be doing anything but you never know...
Also should we eagerly discard any active spans?
There was a problem hiding this comment.
Right, we should probably be able to achieve this by calling the existing doStart/doStop methods on enabled/disabled respectively.
There was a problem hiding this comment.
I think we can't quite do that, doStop() blocks until the shutdown is complete but when disabling it dynamically we shouldn't wait.
|
|
||
| public static final CapturingSpanExporter CAPTURING_SPAN_EXPORTER = new CapturingSpanExporter(); | ||
|
|
||
| static final Setting<Boolean> APM_ENABLED_SETTING = Setting.boolSetting("xpack.apm.enabled", false, Dynamic, NodeScope); |
There was a problem hiding this comment.
I know the other two settings are also under xpack.apm but this name worries me a bit - we're not disabling APM, just tracing ES internals to APM. We should move to a more specific namespace, maybe xpack.tracing.apm_exporter.*. We can do this in a follow-up, no need to combine these changes here.
x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
Show resolved
Hide resolved
# Conflicts: # x-pack/plugin/apm-integration/src/internalClusterTest/java/org/elasticsearch/xpack/apm/ApmIT.java # x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
Show resolved
Hide resolved
DaveCTurner
left a comment
There was a problem hiding this comment.
I left a few more comments.
x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
Show resolved
Hide resolved
| return; | ||
| } | ||
| this.spans.clear();// discard in-flight spans | ||
| shutdownPermits.tryAcquire(); |
There was a problem hiding this comment.
I think this is ok but I would be happier if the services held a permit for their whole lifetime.
Also if tryAcquire returns false then we shouldn't release anything. If we tryAcquire in doStart() then we can just not start anything if the permits are already all taken (which implies that we are stopped)
| this.spans.clear();// discard in-flight spans | ||
| shutdownPermits.tryAcquire(); | ||
| services.provider.shutdown().whenComplete(shutdownPermits::release); | ||
| if (wait) { |
There was a problem hiding this comment.
Could we just move this block into the doStop() method? Seems odd to pass in a flag when we could just write the code.
# Conflicts: # x-pack/plugin/apm-integration/src/main/java/org/elasticsearch/xpack/apm/APMTracer.java
xpack.apm.enabledxpack.apm.tracing.enabled
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM with one suggested assertion
| tracer = openTelemetry.getTracer("elasticsearch", Version.CURRENT.toString()); | ||
| var tracer = openTelemetry.getTracer("elasticsearch", Version.CURRENT.toString()); | ||
|
|
||
| this.services = new APMServices(provider, tracer, openTelemetry); |
There was a problem hiding this comment.
| this.services = new APMServices(provider, tracer, openTelemetry); | |
| assert this.services == false; // ClusterApplierService stops before we do | |
| this.services = new APMServices(provider, tracer, openTelemetry); |
There was a problem hiding this comment.
Do you mean assert this.services == null; or uninitialized before we start?
There was a problem hiding this comment.
bah yeah I meant null sorry
|
@elasticmachine run elasticsearch-ci/part-2 |
adding dynamic setting to enable/disable trace collection