FrankinRUC commented on a change in pull request #217: URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514189067
########## File path: pkg/trace/scheduler_trace_context.go ########## @@ -0,0 +1,186 @@ +/* + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package trace + +import ( + "fmt" + "time" + + "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/ext" + "github.com/uber/jaeger-client-go" +) + +// SchedulerTraceContext manages spans for one trace. +// it only designs for the scheduling process so we keeps the interface simple +type SchedulerTraceContext interface { + // ActiveSpan returns current active (latest unfinished) span in this context object. + // Error returns if there doesn't exist an unfinished span. + ActiveSpan() (opentracing.Span, error) + + // StartSpan creates and starts a new span based on the context state with the operationName parameter. + // The new span is the child of current active span if it exists. + // Or the new span will become the root span of this trace. + StartSpan(operationName string) (opentracing.Span, error) + + // FinishActiveSpan finishes current active span and set its parent as active if exists. + // Error returns if there doesn't exist an unfinished span. + FinishActiveSpan() error +} + +var _ SchedulerTraceContext = &SchedulerTraceContextImpl{} + +// SchedulerTraceContextImpl reports the spans to tracer once they are finished. +// Root span's "sampling.priority" tag will be set to 1 to force reporting all spans if OnDemandFlag is true. +type SchedulerTraceContextImpl struct { + Tracer opentracing.Tracer + SpanStack []opentracing.Span + OnDemandFlag bool +} + +func (s *SchedulerTraceContextImpl) ActiveSpan() (opentracing.Span, error) { + if len(s.SpanStack) == 0 { + return nil, fmt.Errorf("active span is not found") + } + return s.SpanStack[len(s.SpanStack)-1], nil +} + +func (s *SchedulerTraceContextImpl) StartSpan(operationName string) (opentracing.Span, error) { + var newSpan opentracing.Span + if span, err := s.ActiveSpan(); err != nil { + newSpan = s.Tracer.StartSpan(operationName) + if s.OnDemandFlag { + ext.SamplingPriority.Set(newSpan, 1) + } + } else { + newSpan = s.Tracer.StartSpan(operationName, opentracing.ChildOf(span.Context())) + } + s.SpanStack = append(s.SpanStack, newSpan) + return newSpan, nil +} + +func (s *SchedulerTraceContextImpl) FinishActiveSpan() error { + span, err := s.ActiveSpan() + if err != nil { + return err + } + span.Finish() + s.SpanStack = s.SpanStack[:len(s.SpanStack)-1] + return nil +} + +var _ opentracing.Span = &DelaySpan{} + +// DelaySpan implements the opentracing.Span interface. +// It will set the FinishTime field and delay reporting when finished. Review comment: In my opinion, we should keep the wrappers as simple as possible. As you can see, we just integrate the start and finish function with tag settings in all scheduling procedure, like level and info so we don't need to call span.SetTag for recording these common tags every time we create a new span. Keeping the wrappers simple is also important if we want to use custom ScheduelrTracer/TraceContext implementation (if users want to define customized tracing behavior). If we want to implement the DebugWithFilter mode, we have to keep all the span unfinished until we decide whether to report it or not, because many tags are set after getting the results and spans will be reported to jaeger collector once we finish them. It means that we have to use a much more complex implementation for wrapper functions, such as adding a new structure to store these spans. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
