dsmiley commented on code in PR #1884:
URL: https://github.com/apache/solr/pull/1884#discussion_r1315389130
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -239,7 +239,13 @@ private void dispatch(
if (log.isDebugEnabled()) {
log.debug("User principal: {}", request.getUserPrincipal());
}
- TraceUtils.setUser(span, String.valueOf(request.getUserPrincipal()));
+ final String principalName;
+ if (request.getUserPrincipal() != null) {
+ principalName = request.getUserPrincipal().getName();
+ } else {
+ principalName = null;
+ }
+ TraceUtils.setUser(span, String.valueOf(principalName));
Review Comment:
I think we should be setting no "user" at all if there is no user principal
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java:
##########
@@ -201,7 +201,7 @@ private void assertOneSpanIsChildOfAnother(List<SpanData>
finishedSpans) {
assertEquals(child.getTraceId(), parent.getTraceId());
}
- private List<SpanData> getAndClearSpans() {
+ static List<SpanData> getAndClearSpans() {
Review Comment:
This is a hack, calling some method on some other test to manage global
lifecycle of some singleton mock tracer. Seems like we might ideally have a
JUnit Rule to manage trace listening, where we might have this method?
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/BasicAuthIntegrationTracingTest.java:
##########
@@ -99,5 +104,12 @@ public void testSetupBasicAuth() throws Exception {
.build();
req.setBasicAuthCredentials(USER, PASS);
assertEquals(0, req.process(cloudClient, COLLECTION).getStatus());
+
+ var finishedSpans = getAndClearSpans();
+ assertEquals(1, finishedSpans.size());
+ var span = finishedSpans.get(0);
+ assertEquals("post:/cluster/security/authentication", span.getName());
+ assertEquals("solr", span.getAttributes().get(TraceUtils.TAG_USER));
+ assertEquals(List.of("set-user"),
span.getAttributes().get(TraceUtils.TAG_OPS));
Review Comment:
Is it not possible to set the operation name later on with OTEL? The
action/operation is a huge part of what the span *is*; deserves to be a part of
the operation name if we can.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
