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.

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

fix SpanKind type assertion in opentracing bridge #2898

Closed
wants to merge 4 commits into from

Conversation

Prajithp
Copy link

@Prajithp Prajithp commented May 13, 2022

We have system which supports multiple distributed tracing systems. When using opentracing bridge, all the spankinds are created as INTERNAL due to type assertion issue in bridge module.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

Can you fix the tests?

@MrAlias MrAlias added blocked:CLA Waiting on CLA to be signed before progress can be made Skip Changelog PRs that do not require a CHANGELOG.md entry labels May 16, 2022
@Prajithp
Copy link
Author

@dmathieu Done. I have already signed the CLA document.

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #2898 (784aed8) into main (d96e8d2) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2898     +/-   ##
=======================================
- Coverage   76.2%   76.2%   -0.1%     
=======================================
  Files        179     179             
  Lines      11939   11940      +1     
=======================================
- Hits        9103    9102      -1     
- Misses      2596    2598      +2     
  Partials     240     240             
Impacted Files Coverage Δ
bridge/opentracing/bridge.go 62.3% <100.0%> (+<0.1%) ⬆️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

@hanyuancheung
Copy link
Member

Seems like the CLA is not signed yet, could you pls check again and fix it? @Prajithp

@Prajithp
Copy link
Author

@hanyuancheung Tried multiple times by signing the Doc. After clicking the Finish button, page is redirecting back to Github without any messages

@@ -497,8 +498,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.

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 = "".

@MrAlias MrAlias removed the blocked:CLA Waiting on CLA to be signed before progress can be made label May 24, 2022
@dmathieu
Copy link
Member

Closing, as this appears to have been fixed in #3096.

@dmathieu dmathieu closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants