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

[extension/jaegerremotesampling] remove jaeger sampling dependency #36977

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ary82
Copy link

@ary82 ary82 commented Dec 28, 2024

Description

Copy the required code from jaeger to extension/jaegerremotesampling:

  • Modify files to remove dependencies
  • Add required code from plugin/sampling/strategyprovider/static
  • Add required code from cmd/collector/app/sampling to internal

Link to tracking issue

Fixes #36976

Testing

  • Add grpc_handler_test.go from cmd/collector/app/sampling/grpc_handler_test.go
  • Add provider_test.go from plugin/sampling/strategyprovider/static/provider_test.go with plugin/sampling/strategyprovider/static/fixtures

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Copy link

linux-foundation-easycla bot commented Dec 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@ary82 ary82 marked this pull request as ready for review December 29, 2024 14:36
@ary82 ary82 requested a review from a team as a code owner December 29, 2024 14:36
@ary82 ary82 requested a review from jpkrohling December 29, 2024 14:36
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

The organization of the components needs to be improved, it's all over the place. I would recommend

internal/
  source/
    source.go - define interface
    filesource/ - define reading from file
    remotesource/ - a proxy that reads from a remote service
  server/
    grpc/ - grpc server that uses Source
    http/ - http server that uses Source and defines its own JSON model

extension/jaegerremotesampling/options.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/options.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/provider.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/provider.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/model.go Outdated Show resolved Hide resolved
ary82 and others added 2 commits December 29, 2024 22:28
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@yurishkuro
Copy link
Member

lgtm! Let's makes sure all CI checks are green.

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@ary82 ary82 closed this Dec 30, 2024
@ary82 ary82 reopened this Dec 30, 2024
Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@ary82
Copy link
Author

ary82 commented Dec 30, 2024

Sorry, closing was a misclick on Close with comment
The CI showed errors on the linter. Here's what the linter says:

jaegerremotesampling/internal/source/filesource/filesource.go:378:12: Error return value of `enc.Encode` is not checked (errcheck)
	enc.Encode(*s)
	         ^
jaegerremotesampling/internal/source/filesource/filesource.go:380:12: Error return value of `dec.Decode` is not checked (errcheck)
	dec.Decode(&copyValue)
	         ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:71:11: Error return value of `w.Write` is not checked (errcheck)
			w.Write([]byte("bad-content"))
			      ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:85:11: Error return value of `w.Write` is not checked (errcheck)
			w.Write([]byte(*strategy.Load()))
			      ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:512:17: Error return value of `os.WriteFile` is not checked (errcheck)
				os.WriteFile(snapshotFile, strategyJson, 0o644)
				           ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:544:17: Error return value of `os.WriteFile` is not checked (errcheck)
				os.WriteFile(snapshotFile, strategyJson, 0o644)
				           ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:371:21: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	require.NoError(t, os.WriteFile(dstFile, srcBytes, 0o644))
	                  ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:392:21: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	require.NoError(t, os.WriteFile(dstFile, []byte(newStr), 0o644))
	                  ^
jaegerremotesampling/internal/source/filesource/filesource_test.go:469:21: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	require.NoError(t, os.WriteFile(tempFile.Name(), []byte("bad value"), 0o644))
  • For the file permissions in filesource_test.go, should the permissions really be 600?
  • As for the errchecks in filesource in Encode and Decode, these are only used in the test and are safe to be ignored. Also should be moved to the test file(edit: fixed)

@yurishkuro
Copy link
Member

For the file permissions in filesource_test.go, should the permissions really be 600?

600 permission should be fine

As for the errchecks in filesource in Encode and Decode, these are only used in the test and are safe to be ignored. Also should be moved to the test file(edit: fixed)

There's rarely a good reason to ignore errors, even in tests - at some point something might break, and ignored error makes it harder to debug. You can always add require.NoError(t, err) in tests.

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@ary82
Copy link
Author

ary82 commented Dec 30, 2024

Thanks for your help :) Should be fixed now

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
@@ -22,11 +22,11 @@ require (
go.opentelemetry.io/collector/featuregate v1.22.1-0.20241220212031-7c2639723f67
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.27.0
golang.org/x/net v0.33.0
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be changes like this in go mod, they are not related to the pr.

Copy link
Author

Choose a reason for hiding this comment

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

x/net/context was being used in the grpc_handler_test file, which caused this change to go.mod.
Changed to use the stdlib context.

Signed-off-by: Aryan Goyal <137564277+ary82@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove extension/jaegerremotesampling dependency on JAEGER/cmd/collector/app/sampling
3 participants