[exporter/kafka] Add factory options for custom metric and log marshalers #16100

bgranetzke · 2022-11-07T17:12:01Z

Description:
Adding ExportFactory options to add custom log/metric marshalers (similar to existing: WithTracesMarshalers)

Link to tracking Issue: 14514

Testing:
Tests were added for the new options as well as adapting the previous test for WithTracesMarshalers to the new method.

TylerHelmuth · 2022-11-07T17:50:41Z

pinging @pavolloffay @MovieStoreGuy as code owners

bogdandrutu · 2022-11-07T18:46:52Z

Need to fix lint/tests by running make gotidy

MovieStoreGuy

Just a few things I would like addressed just to keep the practices consistent within the component.

MovieStoreGuy · 2022-11-07T23:50:12Z

exporter/kafkaexporter/factory_test.go

+type customTracesMarshaler struct {
+}
+
+var _ TracesMarshaler = (*customTracesMarshaler)(nil)
+
+func (c customTracesMarshaler) Marshal(_ ptrace.Traces, topic string) ([]*sarama.ProducerMessage, error) {
+	panic("implement me")
+}
+
+func (c customTracesMarshaler) Encoding() string {
+	return "custom"
+}
+
+type customMetricsMarshaler struct {
+}
+
+var _ MetricsMarshaler = (*customMetricsMarshaler)(nil)
+
+func (c customMetricsMarshaler) Marshal(_ pmetric.Metrics, topic string) ([]*sarama.ProducerMessage, error) {
+	panic("implement me")
+}
+
+func (c customMetricsMarshaler) Encoding() string {
+	return "custom"
+}


Can you please the declaration above the tests?

I am not a big fan of forward declaration of types and I try keep it inline with how it would be presented with godoc.

To make sure I'm clear, do you want me to:

move all the declarations to the top of the file

move all the declarations before the first marshaler test

move each custom marshaler declaration ahead of its respective test

Just to give a bit of context, I prefer to keep code into logical groupings similar as you would in Java. A class or type per file and an associated test. This mostly help me group logic and behaviour in one spot so that it is really simple for me to glance and know if this is the code I want or not.

To be clear, I not against having shared functionality in file since it can make sense when the logic / types are tightly coupled

What I like to avoid is referencing something before it has been defined, for example:

// foo_test.go func TestMethod(...) { mocker := NewMock(...) } // Additional lines here func NewMock(...) Mock { } type Mock struct {}

So I how I would arrange this is:

// foo_test.go type Mock struct {} func NewMock(...) Mock func TestMethod(...) { mocker := NewMock(..) }

Structuring like above has the added benefit that as I read the file, I can understand what the store state is, what things it depends on, and understand how you're expecting others to use it.

To make sure I'm clear, do you want me to:

move all the declarations to the top of the file

move all the declarations before the first marshaler test

move each custom marshaler declaration ahead of its respective test

Option 2 from your list, I hope my explanation above helps convey what I mean a bit better and why I would like it this way. It is honestly so I don't have to jump around the file to double check how things work or should work.

MovieStoreGuy · 2022-11-07T23:51:11Z

exporter/kafkaexporter/kafka_exporter.go

@@ -173,6 +174,7 @@ func newMetricsExporter(config Config, set component.ExporterCreateSettings, mar
 	if marshaler == nil {
 		return nil, errUnrecognizedEncoding
 	}
+	set.Logger.Info(fmt.Sprintf(logMarshalerUsedMsg, marshaler))


What is the intent of these log lines?

I would encourage the use of structured logging instead of making a called to fmt.Sprintf

Suggested change

set.Logger.Info(fmt.Sprintf(logMarshalerUsedMsg, marshaler))

set.Logger.Debug("Configured marshaller", zap.String("encoding", config.Encoding))

The strategy for the test is to validate the message contents via a zap.Hook. Unfortunately, the fields of the message are not provided in the zapcore.Entry provided to the hook.

I think @MovieStoreGuy is right. Are you sure about "Unfortunately, the fields of the message are not provided in the zapcore.Entry provided to the hook.", does seem like a bug in zap, and hard to believe it.

Oh interesting, ~~I wasn't aware that logs lines were covered as part of the test.~~ It didn't register that was how it was being tested.

From my perspective, I don't see too much value in validating that log lines happened since it often leads into an antipattern of checking if logs exist to see if some functionality has been reached. There should be other state to be able to inspect to validate that it has indeed done what it says it has.

Furthermore, These are more just feedback to the user who provided the configuration. They may or may not find them useful.

As per my comment on structured logging, the reason I'm firm for this is that it makes search logs more efficient in something like loki, splunk, tail -f | grep, pick your poison.

Keeping the message field static means that I can take that text and either include or exclude those lines really easily and those systems can take advantage of their engines that can optimise on that static string. (From my understand, some use bloom filters to make filtering easier).

Finally, not that it is strictly important here but it does remove one additional allocation for the string which may not seem like much but it definitely adds up when you consider all the components and core service of the collector.

MovieStoreGuy · 2022-11-07T23:53:05Z

exporter/kafkaexporter/kafka_exporter.go

@@ -193,6 +195,7 @@ func newTracesExporter(config Config, set component.ExporterCreateSettings, mars
 	if marshaler == nil {
 		return nil, errUnrecognizedEncoding
 	}
+	set.Logger.Info(fmt.Sprintf(logMarshalerUsedMsg, marshaler))


Same as above.

MovieStoreGuy · 2022-11-07T23:53:12Z

exporter/kafkaexporter/kafka_exporter.go

@@ -210,6 +213,7 @@ func newLogsExporter(config Config, set component.ExporterCreateSettings, marsha
 	if marshaler == nil {
 		return nil, errUnrecognizedEncoding
 	}
+	set.Logger.Info(fmt.Sprintf(logMarshalerUsedMsg, marshaler))


Same as above.

MovieStoreGuy · 2022-11-09T11:01:11Z

Hey @bgranetzke ,

This appears to be a duplicate of #13433, is that correct?

bgranetzke · 2022-11-09T12:53:11Z

@MovieStoreGuy You are correct. The addition of the factory options are identical, but the testing approaches are different.

#13433 tried to verify custom marshalers by verifying message output. However, in order to capture the resultant messages, a new abstraction had to be added to the design...which you did not like.

The new approach validates successful injection of the custom marshalers by injecting a log hook into the zap.logger which travels with the exporterSettings. Also not ideal, but the only change to the existing design was the addition of the log message.

TylerHelmuth · 2022-11-10T15:43:01Z

@bgranetzke did you have a bad merge? There are a lot of changed files now.

bgranetzke · 2022-11-10T16:47:47Z

@TylerHelmuth maybe because I rebased against latest main this morning? My source branch still only shows 6 files changed. I try to keep it current with main, but that was probably a bad decision. Any suggestions beyond just recreating the branch?

TylerHelmuth · 2022-11-10T17:02:37Z

@bgranetzke trying rebasing again from upstream/main or doing a merge from upstream/main

MovieStoreGuy · 2022-11-11T11:34:40Z

@MovieStoreGuy You are correct. The addition of the factory options are identical, but the testing approaches are different.

Easy, I thought I was going mad thinking I had already reviewed this.

#13433 tried to verify custom marshalers by verifying message output. However, in order to capture the resultant messages, a new abstraction had to be added to the design...which you did not like.

Yes, sorry about this.
Unfortunately I had some family health issues that I needed to help with and unfortunately dropped the ball on the reviews I was doing.

In future, I would appreciate just pinging me again on the PR if that is okay?

The new approach validates successful injection of the custom marshalers by injecting a log hook into the zap.logger which travels with the exporterSettings. Also not ideal, but the only change to the existing design was the addition of the log message.

I think we are on the same page on it noting being ideal, however, I may have a solution for you.

MovieStoreGuy · 2022-11-11T11:57:21Z

exporter/kafkaexporter/factory_test.go

 	})
 }
+func TestWithMetricsMarshalers(t *testing.T) {


Alright, to help make lives easier on how best to test this, what I suggest you do is the following:

func TestWithMarshalers(t *testing.T) { tests := []struct { name string encoding string marshalers []TracesMarshaler err error }{ { name: "default_encoding", encoding: defaultEncoding, marshalers: nil, err: nil, }, { name: "custom_encoding", encoding: "custom", marshalers: []TracesMarshaler{customMarshaler{}}, err: nil, }, } for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() conf := createDefaultConfig().(*Config) // disable contacting broker conf.Metadata.Full = false conf.Encoding = tc.encoding f := NewFactory(WithTracesMarshalers(tc.marshalers...)) exporter, err := f.CreateTracesExporter( context.Background(), componenttest.NewNopExporterCreateSettings(), conf, ) assert.ErrorIs(t, err, tc.err, "Must match the expected error") if tc.err != nil { assert.Nil(t, exporter, "Must return nil value for invalid exporter") return } assert.NotNil(t, exporter, "Must return valid exporter when no error is returned") }) } }

Structuring the tests like this would mean that it is clear what it trying to validate and only the table value need to change and the rest can remain the same.

I am so sorry that the existing testing doesn't make this easy, however, moving to the above will make it easier for you and those contributing make this component better :)

github-actions · 2022-11-26T05:17:30Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

MovieStoreGuy · 2022-11-29T08:02:10Z

Hey @bgranetzke,

Are you still working on this?

bgranetzke · 2022-11-29T21:47:23Z

@MovieStoreGuy Yes. I'd like to figure out how to finish this PR. Sorry for the delay. I'm paying the price for waiting so long with this current merge. After that I will incorporate your changes. I did figure out how to intercept the structured log fields, but it's a bit more effort than the hooks.

bgranetzke requested review from a team and TylerHelmuth November 7, 2022 17:12

github-actions bot assigned bogdandrutu Nov 7, 2022

bgranetzke mentioned this pull request Nov 7, 2022

exporter/kafkaexporter: Ability to add custom Log and Metric Marshalers #14514

Closed

TylerHelmuth requested review from MovieStoreGuy and pavolloffay November 7, 2022 17:50

bogdandrutu approved these changes Nov 7, 2022

View reviewed changes

bgranetzke requested a review from dmitryax as a code owner November 7, 2022 20:04

bgranetzke requested a review from a team November 7, 2022 20:04

MovieStoreGuy requested changes Nov 7, 2022

View reviewed changes

bgranetzke requested review from djaglowski, dashpole, jmacd, Aneurysm9, jpkrohling, codeboten, mx-psi, jsuereth, punya and arminru as code owners November 10, 2022 13:32

github-actions bot added cmd/configschema configschema command cmd/mdatagen mdatagen command cmd/tracegen examples/demo exporter/alibabacloudlogservice Alibaba components exporter/awscloudwatchlogs awscloudwatchlogs exporter labels Nov 10, 2022

github-actions bot added exporter/awsemf awsemf exporter exporter/awskinesis exporter/awsxray exporter/azuredataexplorer labels Nov 10, 2022

MovieStoreGuy reviewed Nov 11, 2022

View reviewed changes

MovieStoreGuy mentioned this pull request Nov 11, 2022

[kafka Exporter] refactor tests #16265

Merged

github-actions bot added the Stale label Nov 26, 2022

MovieStoreGuy mentioned this pull request Nov 29, 2022

[kafka exporter] Allow for additional marshalers to be added. #16514

Merged

bgranetzke closed this Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/kafka] Add factory options for custom metric and log marshalers #16100

[exporter/kafka] Add factory options for custom metric and log marshalers #16100

bgranetzke commented Nov 7, 2022

TylerHelmuth commented Nov 7, 2022

bogdandrutu commented Nov 7, 2022

MovieStoreGuy left a comment

MovieStoreGuy Nov 7, 2022

bgranetzke Nov 9, 2022

MovieStoreGuy Nov 11, 2022

MovieStoreGuy Nov 11, 2022

MovieStoreGuy Nov 7, 2022

MovieStoreGuy Nov 7, 2022

bgranetzke Nov 9, 2022

bogdandrutu Nov 11, 2022

MovieStoreGuy Nov 11, 2022 •

edited

Loading

MovieStoreGuy Nov 11, 2022

MovieStoreGuy Nov 7, 2022

MovieStoreGuy Nov 7, 2022

MovieStoreGuy commented Nov 9, 2022

bgranetzke commented Nov 9, 2022

TylerHelmuth commented Nov 10, 2022

bgranetzke commented Nov 10, 2022

TylerHelmuth commented Nov 10, 2022

MovieStoreGuy commented Nov 11, 2022

MovieStoreGuy Nov 11, 2022

MovieStoreGuy Nov 11, 2022

github-actions bot commented Nov 26, 2022

MovieStoreGuy commented Nov 29, 2022

bgranetzke commented Nov 29, 2022

	set.Logger.Info(fmt.Sprintf(logMarshalerUsedMsg, marshaler))
	set.Logger.Debug("Configured marshaller", zap.String("encoding", config.Encoding))

[exporter/kafka] Add factory options for custom metric and log marshalers #16100

[exporter/kafka] Add factory options for custom metric and log marshalers #16100

Conversation

bgranetzke commented Nov 7, 2022

TylerHelmuth commented Nov 7, 2022

bogdandrutu commented Nov 7, 2022

MovieStoreGuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy commented Nov 9, 2022

bgranetzke commented Nov 9, 2022

TylerHelmuth commented Nov 10, 2022

bgranetzke commented Nov 10, 2022

TylerHelmuth commented Nov 10, 2022

MovieStoreGuy commented Nov 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2022

MovieStoreGuy commented Nov 29, 2022

bgranetzke commented Nov 29, 2022

MovieStoreGuy Nov 11, 2022 •

edited

Loading