Main otel commit latest search#3
Conversation
Gradle Check (Jenkins) Run Completed with:
|
| } finally { | ||
| if (request.uri().startsWith("/_search")) { | ||
|
|
||
| System.out.println("ending base request:" + request.getRequestId()); |
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * This class encapsulates the state needed to execute a search. It holds a reference to the |
There was a problem hiding this comment.
Please update the javadoc.
| } | ||
|
|
||
| private NoopSpan createNoopSpan(String spanName, Span parentSpan, Level level) { | ||
| logger.info("Starting Noop span name:{}", spanName); |
There was a problem hiding this comment.
Should be a debug log.
| OSSpan parentOSSpan = getLastOSSpanInChain(parentSpan); | ||
| io.opentelemetry.api.trace.Span otelSpan = createOtelSpan(spanName, parentOSSpan); | ||
| Span span = new OSSpan(spanName, otelSpan, parentOSSpan, level); | ||
| logger.info("Starting OtelSpan spanId:{} name:{}: traceId:{}", otelSpan.getSpanContext().getSpanId(), |
There was a problem hiding this comment.
Should be a debug log.
|
|
||
| private void addSingleAttribute(String key, Object value, Span currentSpan) { | ||
| if (currentSpan instanceof OSSpan && ((OSSpan) currentSpan).getOtelSpan() != null) { | ||
| if (value instanceof String) { |
There was a problem hiding this comment.
Switch case if possible.
|
|
||
| private static OSSpan getLastOSSpanInChain(Span span) { | ||
| while (!(span instanceof OSSpan)) { | ||
| span = span.getParentSpan(); |
There was a problem hiding this comment.
Why are you reusing the parameter variable and handle null otherwise null.getParentSpan will throw NPE.
| osSpan.getSpanContext().getTraceId()); | ||
| osSpan.getOtelSpan().end(); | ||
| } else { | ||
| logger.info("Ending noop span name:{}", span.getSpanName()); |
There was a problem hiding this comment.
In case of Noop also shouldn't we setting the Noop's parent as new current?
| } | ||
|
|
||
| private OSSpan getLastOSSpanInChain(Span parentSpan) { | ||
| while (!(parentSpan instanceof OSSpan)) { |
There was a problem hiding this comment.
In case of noop can we add an event to the parent may be that this is mapped to this parent becuase of noop span in-between. Also lets' update the documentation on how level works. May be in the Levels enum.
| this.order = order; | ||
| } | ||
|
|
||
| public boolean isHigherThan(Level level) { |
There was a problem hiding this comment.
Let's keep this logic here. So that we don't replicate the same at multiple places.
| return level; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException("invalid value for tracing level [" + type + "], " + |
There was a problem hiding this comment.
Pls check if this can be validated upfront instead if throwing an exception from here. Also validate if this exception will bring down the ES process?
2 participants
Description
[Describe what this change achieves]
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.