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

Span suppressor #174

Merged
merged 20 commits into from
Dec 16, 2024
Merged

Span suppressor #174

merged 20 commits into from
Dec 16, 2024

Conversation

oliver-zhang
Copy link
Contributor

fixed #125

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2024

CLA assistant check
All committers have signed the CLA.

@123liuziming
Copy link
Collaborator

Grpc has released the latest version 2 days ago, which may cause the CI failed. I will fix the issue as soon as possible in order to let you pass the CI. #173. Thanks for your PR!

@oliver-zhang oliver-zhang changed the title Span supressor Span suppressor Nov 11, 2024
@123liuziming
Copy link
Collaborator

I 've fixed the latestdepth test. You can merge the main branch and trigger the CI again.

@y1yang0
Copy link
Member

y1yang0 commented Nov 13, 2024

TestBasic fails with TestSpanKindShouldSuppressSameKind :

=== RUN   TestSpanKindShouldSuppressSameKind
--- FAIL: TestSpanKindShouldSuppressSameKind (0.00s)
panic: value method github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter.SpanKindSuppressor.StoreInContext called using nil *SpanKindSuppressor pointer [recovered]
	panic: value method github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter.SpanKindSuppressor.StoreInContext called using nil *SpanKindSuppressor pointer

goroutine 40 [running]:
testing.tRunner.func1.2({0x635a00, 0xc000119270})
	/opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1632 +0x230
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1635 +0x35e
panic({0x635a00?, 0xc000119270?})
	/opt/hostedtoolcache/go/1.23.2/x64/src/runtime/panic.go:785 +0x132
github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter.(*SpanKindSuppressor).StoreInContext(0x66abc0?, {0x6ea670?, 0x89[55](https://github.com/alibaba/opentelemetry-go-auto-instrumentation/actions/runs/11789226886/job/32837649062?pr=174#step:5:56)80?}, 0xf?, {0x6ebdc8?, 0xc000100370?})
	<autogenerated>:1 +0x108
github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter.TestSpanKindShouldSuppressSameKind(0xc00016e9c0)
	/home/runner/work/opentelemetry-go-auto-instrumentation/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter/span_suppression_strategy_test.go:65 +0x9b
testing.tRunner(0xc00016e9c0, 0x6a84d8)
	/opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0xf4
created by testing.(*T).Run in goroutine 1
	/opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x390
FAIL	github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/inst-api/instrumenter	0.008s

@123liuziming
Copy link
Collaborator

image
There are still some errors in ut

@oliver-zhang
Copy link
Contributor Author

image There are still some errors in ut

It's work well in my local

=== RUN   TestSemConvShouldNotSuppressContextWithPartiallyDifferentSpanKeys
--- PASS: TestSemConvShouldNotSuppressContextWithPartiallyDifferentSpanKeys (0.00s)
PASS

@123liuziming
Copy link
Collaborator

image There are still some errors in ut

It's work well in my local

=== RUN   TestSemConvShouldNotSuppressContextWithPartiallyDifferentSpanKeys
--- PASS: TestSemConvShouldNotSuppressContextWithPartiallyDifferentSpanKeys (0.00s)
PASS

I will check our CI later.

@123liuziming
Copy link
Collaborator

@oliver-zhang You should run your tests together, the TestSemConvShouldNotSuppressContextWithPartiallyDifferentSpanKeys is infected by other test cases(Because your suppressor is a singleton)
image
image


func (s *SpanKindSuppressor) StoreInContext(context context.Context, spanKind trace.SpanKind, span trace.Span) context.Context {
spanSuppressor, exists := s.delegates[spanKind]
if !exists {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The spankey should be stored into span rather than context. Sometimes users may not deliver the correct go context, at that time, the parentContext in ShouldSuppress may be context.Background(). When the parentContext is context.Background, we will use GLS to fetch the parent context in this Go Routine, we can fetch the parent span and its attributes in GLS, so you should put the span key into span's attributes rather than the go context.

@oliver-zhang oliver-zhang marked this pull request as draft November 21, 2024 01:57
@123liuziming
Copy link
Collaborator

#202 #199 have been merged into main, you can continue to work on this PR based on those PRS

@oliver-zhang
Copy link
Contributor Author

#202 #199 have been merged into main, you can continue to work on this PR based on those PRS

@oliver-zhang
Copy link
Contributor Author

#202 #199 have been merged into main, you can continue to work on this PR based on those PRS

I haven't seen any enhancements to span. I remember that we discussed enhancing span and then put spankey into this attribute to avoid writing unnecessary attributes.

@123liuziming
Copy link
Collaborator

#202 #199 have been merged into main, you can continue to work on this PR based on those PRS

I haven't seen any enhancements to span. I remember that we discussed enhancing span and then put spankey into this attribute to avoid writing unnecessary attributes.

You can use the scope in span to get the span kind. All the scopes are listed in pkg/inst-api/utils/scope.go
image

@123liuziming 123liuziming marked this pull request as ready for review December 3, 2024 04:04
@123liuziming
Copy link
Collaborator

Still some errors in your ut, could you please fix them?

@oliver-zhang oliver-zhang marked this pull request as draft December 3, 2024 06:05
@123liuziming 123liuziming marked this pull request as ready for review December 3, 2024 10:25
@123liuziming 123liuziming merged commit 2be40d8 into alibaba:main Dec 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supress nested client/server span
4 participants