stillalex commented on code in PR #1884:
URL: https://github.com/apache/solr/pull/1884#discussion_r1315875804
##########
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:
re. `op1,op2,op3` I would avoid having the op name dynamic and of unknown
length. it makes its usefulness in searching and grouping almost zero.
> An alternative would be to leave this be but always create another span at
a lower level immediately in front of doing the operation. At least then there
would be a span per operation!
this one makes more sense. will give it a go to see how it looks, but the
added complexity of wrapping each operation in a span might be overkill for
this part of the code. I see 2 aspects here: performance and auditability and
I'm hard pressed to make a good argument for either at this level. but maybe I
am missing something.
--
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
