You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/10/16 09:40:07 UTC

[GitHub] [incubator-yunikorn-core] FrankinRUC opened a new pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

FrankinRUC opened a new pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514609052



##########
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.
+type DelaySpan struct {
+	opentracing.Span
+	FinishTime time.Time
+}
+
+// Finish implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) Finish() {
+	panic("should not call it")
+}
+
+// FinishWithOptions implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) FinishWithOptions(opentracing.FinishOptions) {
+	panic("should not call it")
+}
+
+var _ SchedulerTraceContext = &DelaySchedulerTraceContextImpl{}
+
+// DelaySchedulerTraceContextImpl delays reporting spans
+// and chooses whether to report based on FilterTags when the entire trace is collected.
+type DelaySchedulerTraceContextImpl struct {
+	Tracer     opentracing.Tracer
+	SpanStack  []*DelaySpan
+	Spans      []*DelaySpan
+	FilterTags map[string]interface{}

Review comment:
       If we are setting this via REST API, and pass a flag: 
     abc=1234
   
   how can we know 1234 is a string or an int?
   go-yaml requires us to set up structs before parsing the plain text, I am not quite sure how to set the `map[string]interface{}` in the struct.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-722795830


   I reopen this PR and trigger travis build. The tests are passed this time.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964296


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=h1) Report
   > Merging [#217](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/0ba29ff68f893a509ea355fb9caee42cf2f154b7?el=desc) will **increase** coverage by `0.56%`.
   > The diff coverage is `82.63%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #217      +/-   ##
   ==========================================
   + Coverage   60.94%   61.50%   +0.56%     
   ==========================================
     Files          67       71       +4     
     Lines        5774     5918     +144     
   ==========================================
   + Hits         3519     3640     +121     
   - Misses       2112     2131      +19     
   - Partials      143      147       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/trace\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci90cmFjZV91dGlscy5nbw==) | `39.13% <39.13%> (ø)` | |
   | [pkg/trace/scheduler\_tracer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZXIuZ28=) | `82.92% <82.92%> (ø)` | |
   | [pkg/trace/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3V0aWxzLmdv) | `84.61% <84.61%> (ø)` | |
   | [pkg/trace/scheduler\_trace\_context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZV9jb250ZXh0Lmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `100.00% <0.00%> (+11.11%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=footer). Last update [0ba29ff...6438371](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964296


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=h1) Report
   > Merging [#217](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/0ba29ff68f893a509ea355fb9caee42cf2f154b7?el=desc) will **increase** coverage by `0.49%`.
   > The diff coverage is `82.75%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #217      +/-   ##
   ==========================================
   + Coverage   60.94%   61.43%   +0.49%     
   ==========================================
     Files          67       71       +4     
     Lines        5774     5923     +149     
   ==========================================
   + Hits         3519     3639     +120     
   - Misses       2112     2134      +22     
   - Partials      143      150       +7     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/trace\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci90cmFjZV91dGlscy5nbw==) | `39.13% <39.13%> (ø)` | |
   | [pkg/trace/scheduler\_tracer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZXIuZ28=) | `82.50% <82.50%> (ø)` | |
   | [pkg/trace/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3V0aWxzLmdv) | `84.61% <84.61%> (ø)` | |
   | [pkg/trace/scheduler\_trace\_context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZV9jb250ZXh0Lmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `65.07% <0.00%> (-13.75%)` | :arrow_down: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.19% <0.00%> (+0.08%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=footer). Last update [0ba29ff...0549de8](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964269


   Hey @FrankinRUC,  
   Your changes look good to me!
   
   <a href="https:&#x2F;&#x2F;travis-ci.org&#x2F;apache&#x2F;incubator-yunikorn-core&#x2F;builds&#x2F;736311253">View build log</a>
   
   ###### TravisBuddy Request Identifier: 7600db80-0f9a-11eb-8acb-27a1e4e1eb88
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-714235836


   hi @TaoYang526  could you pls help to review the patch?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-726389526


   Thank you @wilfred-s  for helping to review this!


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964296


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=h1) Report
   > Merging [#217](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/0ba29ff68f893a509ea355fb9caee42cf2f154b7?el=desc) will **increase** coverage by `0.49%`.
   > The diff coverage is `82.75%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #217      +/-   ##
   ==========================================
   + Coverage   60.94%   61.43%   +0.49%     
   ==========================================
     Files          67       71       +4     
     Lines        5774     5923     +149     
   ==========================================
   + Hits         3519     3639     +120     
   - Misses       2112     2134      +22     
   - Partials      143      150       +7     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/trace\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci90cmFjZV91dGlscy5nbw==) | `39.13% <39.13%> (ø)` | |
   | [pkg/trace/scheduler\_tracer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZXIuZ28=) | `82.50% <82.50%> (ø)` | |
   | [pkg/trace/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3V0aWxzLmdv) | `84.61% <84.61%> (ø)` | |
   | [pkg/trace/scheduler\_trace\_context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZV9jb250ZXh0Lmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `65.07% <0.00%> (-13.75%)` | :arrow_down: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.19% <0.00%> (+0.08%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=footer). Last update [0ba29ff...0549de8](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-721601406


   Hi @yangwwei , I add some notes for SchedulerTraceContext and the wrapper. I think we cannot avoid it since we don't have AutoCloseable for spans. We have to finish the spans manually like closing useless resources or flushing buffers.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514948188



##########
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.
+type DelaySpan struct {
+	opentracing.Span
+	FinishTime time.Time
+}
+
+// Finish implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) Finish() {
+	panic("should not call it")
+}
+
+// FinishWithOptions implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) FinishWithOptions(opentracing.FinishOptions) {
+	panic("should not call it")
+}
+
+var _ SchedulerTraceContext = &DelaySchedulerTraceContextImpl{}
+
+// DelaySchedulerTraceContextImpl delays reporting spans
+// and chooses whether to report based on FilterTags when the entire trace is collected.
+type DelaySchedulerTraceContextImpl struct {
+	Tracer     opentracing.Tracer
+	SpanStack  []*DelaySpan
+	Spans      []*DelaySpan
+	FilterTags map[string]interface{}

Review comment:
       I write a demo code for this case:
   ```
   package main
   
   import (
   	"fmt"
   
   	"gopkg.in/yaml.v2"
   )
   
   var data = `
   a: 1
   b: test
   `
   
   func main() {
   	config := make(map[string]interface{})
   	err := yaml.Unmarshal([]byte(data), &config)
   	if err != nil {
   		fmt.Println("unmarshal error")
   	} else {
   		for k, v := range config {
   			fmt.Printf("%s: %v (type: %T)\n", k, v, v)
   		}
   	}
   }
   ```
   And the output will be:
   ```
   a: 1 (type: int)
   b: test (type: string)
   ```




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514177364



##########
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.
+type DelaySpan struct {
+	opentracing.Span
+	FinishTime time.Time
+}
+
+// Finish implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) Finish() {
+	panic("should not call it")
+}
+
+// FinishWithOptions implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) FinishWithOptions(opentracing.FinishOptions) {
+	panic("should not call it")
+}
+
+var _ SchedulerTraceContext = &DelaySchedulerTraceContextImpl{}
+
+// DelaySchedulerTraceContextImpl delays reporting spans
+// and chooses whether to report based on FilterTags when the entire trace is collected.
+type DelaySchedulerTraceContextImpl struct {
+	Tracer     opentracing.Tracer
+	SpanStack  []*DelaySpan
+	Spans      []*DelaySpan
+	FilterTags map[string]interface{}

Review comment:
       Tag value's type can be int or string so we use interface{}. And map[string]interface{} can be parsed by go-yaml or json in these simple case.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514956324



##########
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:
       Do you mean that we should keep a index pointing to the last unfinished (active) span and move it back and forth to find the new  active span when we start or finish spans? The stask is a space-time tradeoff. It records all the unfinished spans to save the time for finding the active span. We know that it will be more space comsuming than regular tracing method, so we only use this implementation if we need the scheduling result tags to find out whether we should collect this trace.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-714244621


   > hi @TaoYang526 could you pls help to review the patch?
   
   Hi, @yangwwei 
   I have reviewed this PR before submitted.  Hope to hear thoughts from you and @wilfred-s , thanks.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964296






----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-721540123


   Thank you @FrankinRUC  for the update, thank you @TaoYang526  for the review.
   I think this can work, and this is more efficient. The only concern I have is, the `ActiveSpan`, `FinishSpan` must be called in pairs, otherwise, it could cause really bad things (not sure how to avoid that though). Can we at least add some notes in the code to explicitly mention that? 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514082157



##########
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 {

Review comment:
       The scheduler currently works in a single thread so we don't need a lock for it. If we use several thread for scheduling, I think we would better maintain one trace context object for each thread.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] wilfred-s closed pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514591991



##########
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 {

Review comment:
       The scheduler is not a single thread service. Because we have multiple goroutines and event handling, many operations are using async mode. But if everything is injected in the `pkg/scheduler` package, it's probably fine. While adding the code in the scheduling core logic, let's keep this in mind.

##########
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:
       If this is the case, could you please take a look at if the implementation can be simplified? Currently, it maintains a `SpanStack  []*DelaySpan` and `Spans []*DelaySpan`. Can we remove one of them, and use a pointer instead?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-714309230


   > I have reviewed this PR before submitted. Hope to hear thoughts from you and @wilfred-s , thanks.
   
   Ah, that's cool. Thank you.
   I will take a look this week, thanks a lot.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-720268682


   Hey @FrankinRUC,  
   Your changes look good to me!
   
   <a href="https:&#x2F;&#x2F;travis-ci.org&#x2F;apache&#x2F;incubator-yunikorn-core&#x2F;builds&#x2F;740756570">View build log</a>
   
   ###### TravisBuddy Request Identifier: 905148b0-1cd5-11eb-916b-c54e98fd930e
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514076978



##########
File path: go.mod
##########
@@ -22,20 +22,24 @@ module github.com/apache/incubator-yunikorn-core
 go 1.12
 
 require (
+	github.com/HdrHistogram/hdrhistogram-go v0.9.0 // indirect

Review comment:
       It's used by jaeger-lib




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-722793691


   Hey @FrankinRUC,  
   Your changes look good to me!
   
   <a href="https:&#x2F;&#x2F;travis-ci.org&#x2F;apache&#x2F;incubator-yunikorn-core&#x2F;builds&#x2F;741790541">View build log</a>
   
   ###### TravisBuddy Request Identifier: 719aea60-1fe2-11eb-9dab-419bf2960c30
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-720949002


   LGTM for the latest commit, @yangwwei  please help to review this, Thanks.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-709964296


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=h1) Report
   > Merging [#217](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/0ba29ff68f893a509ea355fb9caee42cf2f154b7?el=desc) will **increase** coverage by `0.49%`.
   > The diff coverage is `82.75%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #217      +/-   ##
   ==========================================
   + Coverage   60.94%   61.43%   +0.49%     
   ==========================================
     Files          67       71       +4     
     Lines        5774     5923     +149     
   ==========================================
   + Hits         3519     3639     +120     
   - Misses       2112     2134      +22     
   - Partials      143      150       +7     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/trace\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci90cmFjZV91dGlscy5nbw==) | `39.13% <39.13%> (ø)` | |
   | [pkg/trace/scheduler\_tracer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZXIuZ28=) | `82.50% <82.50%> (ø)` | |
   | [pkg/trace/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3V0aWxzLmdv) | `84.61% <84.61%> (ø)` | |
   | [pkg/trace/scheduler\_trace\_context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL3RyYWNlL3NjaGVkdWxlcl90cmFjZV9jb250ZXh0Lmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `65.07% <0.00%> (-13.75%)` | :arrow_down: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.19% <0.00%> (+0.08%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=footer). Last update [0ba29ff...0549de8](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/217?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC closed pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC closed pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC edited a comment on pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC edited a comment on pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#issuecomment-721601406


   Hi @yangwwei , I add some notes for SchedulerTraceContext and the wrapper. I think we cannot avoid it since we don't have AutoCloseable for spans. We have to finish the spans manually like closing useless resources or flushing buffers.
   
   CI smoke test failed on TestConfigScheduler. It's kind of weird because I only add some comments.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] FrankinRUC commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
FrankinRUC commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514078675



##########
File path: pkg/scheduler/trace_utils.go
##########
@@ -0,0 +1,85 @@
+/*
+ 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 scheduler
+
+import (
+	"fmt"
+
+	"github.com/opentracing/opentracing-go"
+
+	"github.com/apache/incubator-yunikorn-core/pkg/trace"
+)
+
+const (
+	LevelKey = "level"
+	PhaseKey = "phase"
+	NameKey  = "name"
+	StateKey = "state"
+	InfoKey  = "info"
+)
+
+// startSpanWrapper simplifies span starting process by integrating general tags' setting.
+// The level tag is required, nonempty and logs span's scheduling level. (root, partition, queue, ...)
+// The phase tag is optional and logs span's calling phase. (reservedAllocate, tryAllocate, allocate, ...)
+// The name tag is optional and logs span's related object's identity. (resources' name or ID)

Review comment:
       We can only set the state and info tags after we get the result of the calling, so they are wrapped in the finishActiveSpanWrapper.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #217: [YUNIKORN-444] Add tracer implementation for scheduling process in the core

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #217:
URL: https://github.com/apache/incubator-yunikorn-core/pull/217#discussion_r514054299



##########
File path: go.mod
##########
@@ -22,20 +22,24 @@ module github.com/apache/incubator-yunikorn-core
 go 1.12
 
 require (
+	github.com/HdrHistogram/hdrhistogram-go v0.9.0 // indirect

Review comment:
       This indirect dependency seems not well maintained.
   Could u pls check which library introduces this? You can find this by go mod graph 

##########
File path: pkg/scheduler/trace_utils.go
##########
@@ -0,0 +1,85 @@
+/*
+ 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 scheduler
+
+import (
+	"fmt"
+
+	"github.com/opentracing/opentracing-go"
+
+	"github.com/apache/incubator-yunikorn-core/pkg/trace"
+)
+
+const (
+	LevelKey = "level"
+	PhaseKey = "phase"
+	NameKey  = "name"
+	StateKey = "state"
+	InfoKey  = "info"
+)
+
+// startSpanWrapper simplifies span starting process by integrating general tags' setting.
+// The level tag is required, nonempty and logs span's scheduling level. (root, partition, queue, ...)
+// The phase tag is optional and logs span's calling phase. (reservedAllocate, tryAllocate, allocate, ...)
+// The name tag is optional and logs span's related object's identity. (resources' name or ID)

Review comment:
       missing annotation for `StateKey` and `InfoKey`

##########
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.
+type DelaySpan struct {
+	opentracing.Span
+	FinishTime time.Time
+}
+
+// Finish implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) Finish() {
+	panic("should not call it")
+}
+
+// FinishWithOptions implements the opentracing.Span interface and panics when calling.
+func (d *DelaySpan) FinishWithOptions(opentracing.FinishOptions) {
+	panic("should not call it")
+}
+
+var _ SchedulerTraceContext = &DelaySchedulerTraceContextImpl{}
+
+// DelaySchedulerTraceContextImpl delays reporting spans
+// and chooses whether to report based on FilterTags when the entire trace is collected.
+type DelaySchedulerTraceContextImpl struct {
+	Tracer     opentracing.Tracer
+	SpanStack  []*DelaySpan
+	Spans      []*DelaySpan
+	FilterTags map[string]interface{}

Review comment:
       the `filterTags` value is `interface{}`
   how can this be passed from REST API or a config file?

##########
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:
       I am trying to understand why we have to create a separate impl for this.
   My understanding is the `startSpanWrapper()`, `finishSpanWrapper()` will be called in the scheduling code. In such case, isn't it simpler just handle the filter in the wrapper? i.e when the tag matches, just ignore creating the span.

##########
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 {

Review comment:
       do we need a lock for this struct?
   it might run into data races if there is a concurrent call on these functions.




----------------------------------------------------------------
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:
users@infra.apache.org