Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix SpanKind type assertion in opentracing bridge #2898

Closed
wants to merge 4 commits into from
Closed

fix SpanKind type assertion in opentracing bridge #2898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf1d89d
fix SpanKind type assertion
Prajithp May 13, 2022
48833d7
fix tests
Prajithp May 17, 2022
24b95cb
Merge branch 'main' into fix/bridge
hanyuancheung May 31, 2022
784aed8
Merge branch 'main' into fix/bridge
hanyuancheung Aug 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions bridge/opentracing/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"fmt"
"net/http"
"reflect"
"strings"
"sync"

Expand Down Expand Up @@ -502,8 +503,9 @@ func otTagsToOTelAttributesKindAndError(tags map[string]interface{}) ([]attribut
for k, v := range tags {
switch k {
case string(otext.SpanKind):
if s, ok := v.(string); ok {
switch strings.ToLower(s) {
refValue := reflect.ValueOf(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid using the reflect package, how about taking the opposite approach, of turning strings into SpanKindEnum?

case string(otext.SpanKind):
	if s, ok := v.(string); ok {
		v = otext.SpanKindEnum(strings.ToLower(s))
	}
	if s, ok := v.(otext.SpanKindEnum); ok {
		switch s {
		case otext.SpanKindRPCClientEnum:
			kind = trace.SpanKindClient
		case otext.SpanKindRPCServerEnum:
			kind = trace.SpanKindServer
		case otext.SpanKindProducerEnum:
			kind = trace.SpanKindProducer
		case otext.SpanKindConsumerEnum:
			kind = trace.SpanKindConsumer
		}
	}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks good to me if reflect package is not the ideal solution for this. Should I update the commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for another opinion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanyuancheung @MrAlias

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that reflect seems out of place. I would maybe suggest a map[otext.SpanKindEnum]trace.SpanKind with a default so we don't leave a kind = "".

if refValue.Kind() == reflect.String {
switch strings.ToLower(refValue.String()) {
dmathieu marked this conversation as resolved.
Show resolved Hide resolved
case "client":
kind = trace.SpanKindClient
case "server":
Expand Down
3 changes: 2 additions & 1 deletion bridge/opentracing/mix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

ot "github.com/opentracing/opentracing-go"
"github.com/opentracing/opentracing-go/ext"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
Expand Down Expand Up @@ -637,7 +638,7 @@ func runOtelOTOtel(t *testing.T, ctx context.Context, name string, callback func

func runOTOtelOT(t *testing.T, ctx context.Context, name string, callback func(*testing.T, context.Context) context.Context) {
tr := otel.Tracer("")
span, ctx := ot.StartSpanFromContext(ctx, fmt.Sprintf("%s_OT_OtelOT", name), ot.Tag{Key: "span.kind", Value: "client"})
span, ctx := ot.StartSpanFromContext(ctx, fmt.Sprintf("%s_OT_OtelOT", name), ot.Tag{Key: "span.kind", Value: ext.SpanKindRPCClient.Value})
defer span.Finish()
ctx = callback(t, ctx)
func(ctx2 context.Context) {
Expand Down