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

Removes deprecated ErrNilNextConsumer and solves type error for typeStr #4250

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

tarsojabbes
Copy link
Contributor

@tarsojabbes tarsojabbes commented Apr 4, 2024

This PR corrects type error for typeStr on Custom Receiver documentation, as well as removes ErrNilNextConsumer checks.

As of version 0.97.0 for dependencies when creating a custom receiver, using typeStr directly as a string was throwing an error of cannot use typeStr (variable of type string) as component.Type value in argument to receiver.NewFactory which should now be done using component.MustNewType("tailtracer"), in a var declaration.

Also, as in Issue 9322 and later on accept as PR 9779 and PR 31793, component.ErrNilNextConsumer is not a valid error type.

@tarsojabbes tarsojabbes requested review from a team and Aneurysm9 and removed request for a team April 4, 2024 11:39
Copy link

linux-foundation-easycla bot commented Apr 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

@open-telemetry/collector-contrib-approvers PTAL

Comment on lines 400 to 403
const (
typeStr = "tailtracer"
var (
typeStr = component.MustNewType("tailtracer")
defaultInterval = 1 * time.Minute
)
Copy link
Member

Choose a reason for hiding this comment

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

nit: We could leave defaultInterval as const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Can you take a second look?

@mjnowen
Copy link

mjnowen commented Apr 8, 2024

+1 this PR LGTM

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@cartermp cartermp merged commit d7f2e27 into open-telemetry:main Apr 8, 2024
14 checks passed
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.

8 participants