SOLR-18124: Add tracing spans for UpdateLog replay#4216
SOLR-18124: Add tracing spans for UpdateLog replay#4216openworld-maker wants to merge 4 commits intoapache:mainfrom
Conversation
|
Ran local checks after wiring Java 21 in my env:
Both passed. Happy to adjust naming/attributes if there’s a preferred tracing convention for replay spans. |
| public static String LOG_FILENAME_PATTERN = "%s.%019d"; | ||
| public static String TLOG_NAME = "tlog"; | ||
| public static String BUFFER_TLOG_NAME = "buffer.tlog"; | ||
| private static final String UPDATELOG_REPLAY_SPAN_NAME = "updatelog.replay"; |
There was a problem hiding this comment.
honestly just inline these. The constants serve no purpose. I know this is a matter of taste. The practice of constants spreads readability around thus reducing readability.
| span.setAttribute("updatelog.replay.active_log", activeLog); | ||
| span.setAttribute("updatelog.replay.in_sorted_order", inSortedOrder); | ||
| span.setAttribute("updatelog.replay.logs_total", initialLogCount); | ||
| span.setAttribute("updatelog.replay.core", req.getCore().getName()); |
There was a problem hiding this comment.
Probably remove, assuming redundant with the line below
| TraceUtils.setDbInstance(span, req.getCore().getName()); | ||
| }); | ||
|
|
||
| try (Scope scope = replaySpan.makeCurrent()) { |
There was a problem hiding this comment.
can we avoid adding a try-finally when I see one here already?
| return replayedOps; | ||
| } | ||
|
|
||
| private String summarizeProcessorChain(UpdateRequestProcessorChain processorChain) { |
There was a problem hiding this comment.
lets put this on UpdateRequestProcessorChain.toString().
BTW processorChain won't be null
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
|
|
||
| public class UpdateLogReplayTracingTest extends SolrTestCaseJ4 { |
There was a problem hiding this comment.
Thanks.... I would've been sufficiently happy to see a pic of it working in your tracing viewer of choice. We mostly don't test logs; traces are a glorified log in the end, and thus I think testing traces is questionable value trade-off.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
you added changes here totally unrelated to the issue/description. Looks related to another PR you are working on. Please don't contribute changes to AGENTS.md nor add new md files.
3 participants
Description
This PR addresses SOLR-18124 by adding tracing spans around UpdateLog replay/recovery.
What changed
updatelog.replay.updatelog.replay.log.state,active_log,in_sorted_order)UpdateLogReplayTracingTestto verify:Notes
TraceUtils.Testing
./gradlew :solr:core:test --tests org.apache.solr.update.UpdateLogReplayTracingTest