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

[extension/jaegerremotesampling] remove jaeger sampling dependency #36977

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

[extension/jaegerremotesampling] remove jaeger sampling dependency #36977

wants to merge 16 commits into from
+1,558 −130

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

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Dec 28, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

ary82 added 4 commits December 29, 2024 15:59
@ary82
dependency fixes
94c5d1f 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
quickfix
57479e2 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
change NewProvider to private
fe2045a 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
add changelog
877511a 
Signed-off-by: Aryan Goyal <[email protected]>
@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
yurishkuro
yurishkuro reviewed Dec 29, 2024
View reviewed changes
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
...rremotesampling/fixtures/TestServiceNoPerOperationStrategiesDeprecatedBehavior_ServiceA.json Outdated Show resolved Hide resolved
ary82 and others added 2 commits December 29, 2024 22:28
@yurishkuro
Copy link
Member

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

@ary82 ary82 closed this Dec 30, 2024
@ary82 ary82 reopened this Dec 30, 2024
@ary82
move deepCopy to test file
cb8d0ff 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
Copy link
Author

ary82 commented Dec 30, 2024 •
edited
Loading

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.

@ary82
Copy link
Author

ary82 commented Dec 30, 2024

Thanks for your help :) Should be fixed now

@ary82
run gomod tidy
41ca9c2 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
fix: go.mod
27239ae 
Signed-off-by: Aryan Goyal <[email protected]>
@ary82
Copy link
Author

ary82 commented Jan 7, 2025

Hey @yurishkuro, just checking if this needs any other fix from my side or are we waiting for the other reviewers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@frzifus frzifus Awaiting requested review from frzifus

@jpkrohling jpkrohling Awaiting requested review from jpkrohling jpkrohling is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

At least 1 approving review is required to merge this pull request.

Assignees

@codeboten codeboten

Labels
extension/jaegerremotesampling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove extension/jaegerremotesampling dependency on JAEGER/cmd/collector/app/sampling
3 participants
@ary82 @yurishkuro @codeboten