Provide tracing implementation using OpenTelemetry + APM agent #88443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove unused vars, fix forbidden APIs.
pugnascotia
added
>feature
WIP
:Core/Infra/Core
21 tasks
|
@original-brownbear The test I can un-break the test by reverting the change to For background about what's going in in this PR, see |
Instead of wrapping RequestHandlerRegistry#processMessageReceived(...) in a new trace context, perform the wrapping where that method is calling. This allows handling when `processMessageReceived` throws an exception to use the same context.
Instead of messing around with supporting tracing methods directly on a `RestChannel`, construct a `RestController` with a `Tracer` and trace requests the usual way. As a result, strip out added code from `RestChannel` and `HttpTracer`. Also add Javadoc to `Tracer`.
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java
Outdated
Show resolved
Hide resolved
modules/apm/src/main/java/org/elasticsearch/tracing/apm/APMTracer.java
Outdated
Show resolved
Hide resolved
pugnascotia
added a commit
that referenced
this pull request
Aug 2, 2022
Part of #84369. Split out from #88443. This PR wraps parts logic in `InternalExecutePolicyAction` in a new tracing context. This is necessary so that a tracing implementation can use the thread context to propagate tracing headers, but without the code attempting to set the same key twice in the thread context, which is illegal.
pugnascotia
added a commit
that referenced
this pull request
Aug 2, 2022
Part of #84369. Split out from #88443. This PR wraps parts logic in `AsyncTaskManagementService` in a new tracing context. This is necessary so that a tracing implementation can use the thread context to propagate tracing headers, but without the code attempting to set the same key twice in the thread context, which is illegal.
pugnascotia
added a commit
that referenced
this pull request
Aug 2, 2022
Part of #84369. Split out from #88443. This PR wraps parts logic in `TransportSubmitAsyncSearchAction` in a new tracing context. This is necessary so that a tracing implementation can use the thread context to propagate tracing headers, but without the code attempting to set the same key twice in the thread context, which is illegal.
pugnascotia
added a commit
that referenced
this pull request
Aug 3, 2022
Part of #84369. Split out from #88443. This PR wraps parts of the code in a new tracing context. This is necessary so that a tracing implementation can use the thread context to propagate tracing headers, but without the code attempting to set the same key twice in the thread context, which is illegal. In order to avoid future diff noise, the wrapped code has mostly been refactored into methods. Note that in some places we actually clear the tracing context completely. This is done where the operation to be performed should have no association with the current trace context. For example, when creating a new index via a REST request, the resulting background tasks for the index should not be associated with the REST request in perpetuity.
jpountz
reviewed
Aug 3, 2022
| sources][agent-config], with a hierarchy that means, for example, that options | ||
| set in the config file cannot be overridden via system properties. | ||
|
|
||
| Now, in order to send tracing data to the APM server, ES needs to configured with |
There was a problem hiding this comment.
I think you meant "needs to be configured"?
|
Wonderful! Should we add the |
I'm not honestly sure. We haven't written externally-facing docs yet, as we're focussed on using it for ourselves right now. I'll leave that up to the Core/Infra team. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #84369. Split out from #87696. Implement the
Tracerinterface by providing an X-Pack module that uses OpenTelemetry, along with Elastic's APM agent for Java.See the file
TRACING.mdfor background on the changes and the reasoning for some of the implementation decisions.The configuration mechanism is the most fiddly part of this PR. The Security Manager permissions required by the APM Java agent make it prohibitive to start an agent from within Elasticsearch programmatically, so it must be configured when the ES JVM starts. That means that the startup CLI needs to assemble the required JVM options.
To complicate matters further, the APM agent needs a secret token in order to ship traces to the APM server. We can't use Java system properties to configure this, since otherwise the secret will be readable to all code in Elasticsearch. It therefore has to be configured in a dedicated config file. This in itself is awkward, since we don't want to leave secrets in config files. Therefore, we pull the APM secret token from the keystore, write it to a config file, then delete the config file after ES starts.
There's a further issue with the config file. Any options we set in the APM agent config file cannot later be reconfigured via system properties, so we need to make sure that only "static" configuration goes into the config file.
I generated most of the files under
qa/apmusing an APM test utility (I can't remember which one now, unfortunately). The goal is to setup up a complete system so that traces can be captured in APM server, and the results in Elasticsearch inspected.