SOLR-15367 [9.x] Convert "rid" functionality into a default Tracer #1868
Conversation
|
@dsmiley raising this PR for a discussion on building an OpenTracing version of the always-on tracer that went into main branch based on OTEL. |
solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java
Outdated
Show resolved
Hide resolved
| if (!format.equals(Format.Builtin.HTTP_HEADERS)) { | ||
| throw new IllegalArgumentException("unsupported format " + format); | ||
| } |
There was a problem hiding this comment.
Is this constraint necessary? What if we inject & extract to transfer to the Overseer? (I have done this at work)
There was a problem hiding this comment.
honestly not sure how to approach this so it works on a generic case. I will return change to early in case this is not a known format (the one Solr sets up) instead of throwing.
There was a problem hiding this comment.
In practice at the moment, the format will always be HTTP since we control where we inject. But if we get to add the Overseer as you have done in main branch, then it will mysteriously not propagate, and then maybe we'll "fix" it by removing these 3 lines. I don't think need to care about what the format is; just try to inject best-effort.
There was a problem hiding this comment.
I don't think need to care about what the format is; just try to inject best-effort.
I agree. the inject is easy enough, you just return early. the extract is now a return NoopSpan.INSTANCE.context(); and this I'm not sure will work correctly.
not having some basic test coverage for this setup worries me a bit.
solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java
Outdated
Show resolved
Hide resolved
stillalex
force-pushed
the
SOLR-15367-9x-tracer
branch
from
b640bb6 to
f3cb607
Compare
stillalex
force-pushed
the
SOLR-15367-9x-tracer
branch
from
c7123a8 to
3eb477e
Compare
|
@dsmiley would like to merge this soon. do you think the context injection/extraction parts are correct now? |
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java
Show resolved
Hide resolved
|
thank you @dsmiley I really appreciate your diligence here. very happy to see this land on 9.x too! |
https://issues.apache.org/jira/browse/SOLR-15367
Description
PR for 9.x branch
This is a 9.x version of #1854 for evaluation.
Almost all changes relevant to OpenTracing are in SimplePropagator. I made an effort to minimize changes that are not on main branch. ideally we'd reach a state where this has working basic generation of traces and remains frozen on 9.x until 10.x starts releasing and everyone transitions automatically to the new OTEL-based impl.
Similar to main branch, this is enabled by default. this could be an issue for setups that already have tracing enabled, so I am ok with off by default if switching this off via system property is too much to handle in a minor upgrade.
Solution
Please provide a short description of the approach taken to implement your solution.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.