-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Tracing and log correlation without otel #43718
base: main
Are you sure you want to change the base?
Conversation
Fallback tracing benchmarks
So with fallback tracing (no logs) enabled by default, we're going to create ~2 spans for client SDK code in common case (1 public API-surface and 1 HTTP), adding ~90ns to each client call along with some allocations which seems reasonable. This time is essentially spent on generating random Ids and we'd probably get a comparable impact from x-ms-client-id thing. |
API change check API changes are not detected in this pull request. |
Champion scenario: var httpPipeline = new HttpPipelineBuilder()
.policies(new HttpRetryPolicy(), new HttpRedirectPolicy(), new HttpInstrumentationPolicy(null, null))
.build();
// this would effectively be done by codegen in client libs (when they create a span)
// but users can also provide context explicitly and we'll use it.
// we also support creating this context from OTel spans
RequestOptions requestOptions = new RequestOptions().setInstrumentationContext(createRandomContext());
HttpRequest request = new HttpRequest(HttpMethod.GET, "http://www.microsoft.com")
.setRequestOptions(requestOptions);
httpPipeline.send(request); logs with simulated error (formatted, verbose)
or with simulated error and redirect (formatted, info)
So We can easily extend it to support implicit context propagation (which already works with otel). |
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.
moved to HttpInstrumentationLoggingTests
@@ -314,27 +311,6 @@ public void tracingIsDisabledOnInstance() throws IOException { | |||
assertEquals(0, exporter.getFinishedSpanItems().size()); | |||
} | |||
|
|||
@Test | |||
public void tracingIsDisabledOnRequest() throws IOException { |
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.
removed disabling individual requests - we didn't advertise this feature in azure-core and I don't recall anyone asking for it.
We also don't support it in other languages.
private static final int TRACE_ID_HEX_LENGTH = 32; | ||
private static final int SPAN_ID_HEX_LENGTH = 16; | ||
private static final char[] ENCODING = buildEncodingArray(); | ||
private static final String ALPHABET = "0123456789abcdef"; |
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.
Since this is only used in the utility method buildEncodingArray
could this be turned into a local variable there?
@@ -184,6 +184,7 @@ public static void runtimeError(ClientLogger logger, Throwable t) { | |||
* @return true if OTel is initialized successfully, false otherwise | |||
*/ | |||
public static boolean isInitialized() { | |||
return INSTANCE.initialized; | |||
return INSTANCE == null || INSTANCE.initialized; |
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.
Why does isInitialized
return true if the instance is null?
boolean isAllZero = true; | ||
for (int i = 3; i < 35; i++) { | ||
char c = traceparent.charAt(i); | ||
if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f'))) { |
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.
Is this character range a specification somewhere that could be included here? I know many times both a-f and A-F are acceptible.
for (int i = 0; i < 2; i++) { | ||
if (traceparent.charAt(i) != '0') { | ||
return false; | ||
} | ||
} |
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.
Let's just unroll this loop
if (traceparent.charAt(0) != '0' || traceparent.charAt(1) != '0') {
return false;
}
with that this can then be combined with the next if
if (traceparent.charAt(0) != '0' || traceparent.charAt(1) != '0' || traceparent.charAt(2) != '-') {
return false;
}
if (traceparent.charAt(35) != '-') { | ||
return false; | ||
} |
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.
Thoughts on making the checks non-sequential for better performance?
This and the charAt(52)
checks could be done eagerly to prevent needing to track more information earlier.
Also, somewhere in this class mind adding a comment that outlines the definition of a valid traceparent?
private FallbackSpanContext() { | ||
this.traceId = INVALID_TRACE_ID; | ||
this.spanId = INVALID_SPAN_ID; | ||
this.traceFlags = "00"; | ||
this.isValid = false; | ||
this.span = Span.noop(); | ||
} |
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.
Do we need this constructor if its only usage is to create the static INVALID
? Could that just call the constructor below.
The next PR in the series of client core tracing+logging improvements.
In this episode, we:
InstrumentationContext
- strongly typed container for trace context that's used to correlate logs with or without otelHttpInstrumentationPolicy
withHttpLoggingPolicy
. This is not strictly necessary, but allows toHttpRetryPolicy
andHttpRedirectPolicy
logging
intoinstrumentation
packageIn next episodes:
HttpLogOptions
andInstrumetnationOptions
?