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