[exporter/kafka] support custom metrics & logs marshalers #8248
Conversation
|
@faceair could you include unit tests for the newly added functions? |
|
Hi @pmm-sumo , I have added unit tests, please help review again. |
| cfg.Metadata.Full = false | ||
|
|
||
| t.Run("custom_encoding", func(t *testing.T) { | ||
| cfg.Encoding = cm.Encoding() |
There was a problem hiding this comment.
I think this tests actually are only somewhat useful. They only check that Encoding() function yields a result and factory accepts WithLogsMarshalers/WithMetricMarshalers.
TBH, the currently existing test (the only one which calls WithTracesMarshallers) is not very useful either for the same reasons.
Perhaps that's OK (what's your guidance on this @MovieStoreGuy @pavolloffay?). I was thinking that maybe some sorts of tests matching non default marshaler encoding could be created instead, but perhaps it's too much of a stretch for a relatively simple API change here
There was a problem hiding this comment.
Apologies, didn't mean to leave this for so long. Github notifications are not my friend for some time now.
Currently late evening atm, so I come to review back tomorrow to full understand what is happening.
There was a problem hiding this comment.
Yes, I agree with @pmm-sumo.
The minimal required testing of custom encodings is that the encoded data matches a known output.
Testing that the set encoding matches the expected result is just the start.
|
I have a use case to plug custom metric/log marshalers as well (keyed messages). Thank you for the PR. |
|
I think the linter just wants the }'s on a new line like the prior code: However, I'm struggling to associate the changes in this PR to something that might consume more memory for TestMetricsFromFile`` |
There was a problem hiding this comment.
There is a few things that I care about:
- Return an error in tests instead of panicing (avoid flakey tests and follow contribution guide)
- Further details on how an end user would configure this
- Update the KafkaReceiver to also include additional log and metric marshalers (Haven't verified myself if it does)
- Include validate on the data produced by custom encoding within tests.
Once these items are addressed, I am happy to approve this PR :)
| // WithMetricsMarshalers adds metricsMarshalers. | ||
| func WithMetricsMarshalers(metricsMarshalers ...MetricsMarshaler) FactoryOption { |
There was a problem hiding this comment.
I don't see this being references anywhere within that would allow for these to ether be added by default or added via some configuration.
Can I ask how you'd want someone to use this in their exporter?
There was a problem hiding this comment.
@MovieStoreGuy I think the question for how to use these factory options predates this PR. The WithTraceMarshalers() option existed before this PR. I've attempted 2 methods to utilize the custom options:
- pass in explicit options to NewFactory() in components.go. However, this isn't supported with the collector-builder and I could no longer use it
- create a custom exporter factory where the NewFactory() just delegates to kafkaexporter.NewFactory() with additional options
Taking a step back, are you aware of precedence for extending subcomponents of exporters/receivers? At a high level, extensions for auth looked close, but when I looked deeper the requirements for high-level abstractions to exist in the collector didn't feel right either (e.g go.opentelemetry.io/collector/config/configauth). I know you are aware #8272 which would have been less messy if custom marshalers could be more easily added.
| var _ TracesMarshaler = (*customTracesMarshaler)(nil) | ||
|
|
||
| func (c customTracesMarshaler) Marshal(_ pdata.Traces, topic string) ([]*sarama.ProducerMessage, error) { | ||
| panic("implement me") |
There was a problem hiding this comment.
Not exactly sure why this should panic in test code, what are you trying to achieve here?
| var _ LogsMarshaler = (*customLogsMarshaler)(nil) | ||
|
|
||
| func (c customLogsMarshaler) Marshal(logs pdata.Logs, topic string) ([]*sarama.ProducerMessage, error) { | ||
| panic("implement me") |
There was a problem hiding this comment.
I really worry about panicing in tests, it isn't go idiomatic and not discouraged by the contribution guide.
Can I ask what you're wanting to achieve here?
| cfg.Metadata.Full = false | ||
|
|
||
| t.Run("custom_encoding", func(t *testing.T) { | ||
| cfg.Encoding = cm.Encoding() |
There was a problem hiding this comment.
Yes, I agree with @pmm-sumo.
The minimal required testing of custom encodings is that the encoded data matches a known output.
Testing that the set encoding matches the expected result is just the start.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
I'm trying to use opentelemetry-collector to collect logs, and the format of the logs input to kafka is our internal format, so I need the kafka exporter to support the customization as well.
The kafka exporter already supports custom marshaler for trace, so adding a new marshaler for log & metrics should do the same.