-
Notifications
You must be signed in to change notification settings - Fork 628
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
SOLR-16536 Replace OpenTracing instrumentation with OpenTelemetry #1841
Conversation
Hi, please connect this PR to SOLR-16536 instead (and not the umbrella issue). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge PR, but I think I followed along.
Main comment is whether we should kill TracerConfigurator
and initialize tracer statically.
Rest are a bunch of questions. Have run the unit tests, but not spun up a cluster to compare new spans with old ones.
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java
Outdated
Show resolved
Hide resolved
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/util/tracing/HttpServletRequestGetter.java
Show resolved
Hide resolved
This hits at changes needed in gradle build. There are |
These files from |
you are right. renamed the PR to reflect the correct issue |
Thank you @janhoy for the review! I think the biggest question was the removal of span.log. it felt strange to have logs mixed with spans. maybe I misunderstood the use. happy to look into otel alternatives I am seeing some backup related tests failing on my local and on the crave build. will dig into those a bit more, I want to reach a stable state before making more changes. |
2c07483
to
7566e73
Compare
sorry, had to do a rebase on top of main branch to fix some conflicts. |
quick comparison of Query span data (exporting from jaeger on 9.3 vs this PR): Query spans on this PR
vs Query spans on Solr 9.3
Diff:
|
quick comparison of Indexing span data (exporting from jaeger on 9.3 vs this PR): Update spans on this PR
Update spans on Solr 9.3 PR
Diff:
|
Please consult the otel semantic conventions for what attribute names to use. The former correct tag was Perhaps we should emit both for some versions, or implement the suggested env var |
if this is for 2 fields only ( |
I'm ok with duplicating tags. But we should probably (now or as a follow-up) review every tag name that we explicitly define in our code, to see how it aligns with the recommendations, to make sure we covered all of them. |
Tests seem to fail with |
I reproduced the thread leak locally, but it does not happen every time. Also, the new Crave test run was successful. Looks like there is a timing issue. @stillalex how is the tracer life cycle handled now that we don't explicitly shut down the tracer in |
...s/opentelemetry/src/test/org/apache/solr/opentelemetry/CustomTestOtelTracerConfigurator.java
Outdated
Show resolved
Hide resolved
Wrt documentation. As long as the user-facing config and env vars are the same, and the spans created are almost identical, I think the only Ref-guide documentation to add is in "Major changes in Solr 10" section of upgrade notes, we can mention the deprecation of If we plan to keep sending both for a long time (e.g. 9.x and 10.x) then we're fine and would not need to implement
Would you like to give the docs a shot @stillalex ? Also, please add a line to |
@janhoy updated, please take a look. Just to run one idea by you: the otel library upgrade is causing a lot of conflicts, and in itself it would be good to have it on 9.x branch too. what do you think about splitting only the lib upgrade to a different PR which can be simple and merged to 9.x branch? |
quick smoke test on benchmarking side, I tried running the existing
|
Yes, splitting out the otel version upgrade is a good idea! |
moved the simple otel update to #1846 will rebase once that is in |
2b0d4ed
to
b6e7c75
Compare
@janhoy otel upgrade split, merged and rebase completed. running checks now to verify if I missed anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now. Tests are hopefully stable. Great work!
PS: Did you do a broader review of tag name changes apart from the two we covered, or should we put it up as a new JIRA?
No review yet. I was thinking I could do one as part of SOLR-16935 because I wanted to add 2 more spans. |
https://issues.apache.org/jira/browse/SOLR-16536
Description
Move all tracing code to OTEL.
Solution
TraceUtils
utils class.Open items:
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:
main
branch../gradlew check
.