-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[receiver/pulsar] do not expose method #27029
Conversation
component: pulsarreceiver | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Do not export the function `WithLogsUnmarshalers`, `WithMetricsUnmarshalers`, `WithTracesUnmarshalers` and pass checkapi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removing the methods altogether, not just making them private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i wrote an incorrect description, thanks for reviews @codeboten!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
receiver/pulsarreceiver/factory.go
Outdated
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this removal. This can be actually used along with TracesUnmarshaler
, FactoryOption
and others. Let's start with deprecating them and see it people complain. Please create a separate issue for this removal where potential users can comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, i made a mistake again and accidentally deleted a feature with FactoryOption
. Thanks for reviews and thoughtful suggestion @dmitryax!
@@ -26,33 +26,6 @@ const ( | |||
// FactoryOption applies changes to PulsarExporterFactory. | |||
type FactoryOption func(factory *pulsarReceiverFactory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atoulme, looks like the checkapi
tool missed FactoryOption
, TracesUnmarshaler
, MetricsUnmarshaler
and LogsUnmarshaler
. Is this expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is. For now, the checkapi tool only checks functions, not types. We can add that next but it's more complicated, as Config structs must be exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
**Description:** Do not export functions `WithLogsUnmarshalers`, `WithMetricsUnmarshalers`, `WithTracesUnmarshalers`, add tests for that and pass checkapi **Link to tracking Issue:** open-telemetry#26304 **Testing:** go run cmd/checkapi/main.go . make chlog-validate go test for pulsarreceiver **Documentation:**
Description:
Do not export functions
WithLogsUnmarshalers
,WithMetricsUnmarshalers
,WithTracesUnmarshalers
, add tests for that and pass checkapiLink to tracking Issue:
#26304
Testing:
go run cmd/checkapi/main.go .
make chlog-validate
go test for pulsarreceiver
Documentation: