[receiver/kafkareceiver] Feature/kafkareceiver avro #21068

vincentfree · 2023-04-19T05:58:22Z

Description: Adding avro encoding for kafkareceiver.

Link to tracking Issue: #21067

Testing: added basic test for the new functionality and new yaml config structures

Documentation: added documentation for new config items and an extra example showing how to use Avro structured logging with field mapping and schema referencing

linux-foundation-easycla · 2023-04-19T05:58:27Z

The committers listed above are authorized under a signed CLA.

✅ login: thmshmm / name: Thomas Hamm (f7aef95, 250a3d4)
✅ login: vincentfree / name: Vincent Free (9cca220, f058d63, 47b5a99, 0ce8035, 112b272, f01316e, 0388f54)

atoulme

Those changes look fantastic. Would you be open to becoming a codeowner of this code specifically so we can use your expertise to maintain it going forward?

thmshmm · 2023-04-21T07:42:41Z

Sounds good. Only for the avro part?

atoulme · 2023-04-21T07:56:51Z

Yes? I’m offering but it’s not really up to me. The existing codeowners will weigh in. Given the complexity of the code, I am sure they would be more comfortable with it if you stick around.

thmshmm · 2023-04-21T08:40:21Z

Yes of course.

atoulme · 2023-04-22T22:40:36Z

Please add yourself in .github/CODEOWNERS for the file specifically related to avro, and rebase this PR off the latest main to pick up a fix that is breaking integration tests.

thmshmm · 2023-04-24T08:29:37Z

@atoulme do you know how it is possible to specify ownership for single files?
The test fails
"receiver/kafkareceiver/avro_unmarshaler.go" does not exist as specified in CODEOWNERS

atoulme · 2023-04-24T16:34:40Z

Oh just run make generate-gh-issue-templates we generate a dropdown from codeowners values for github issues and the PR doesn't like that this is out of sync.

thmshmm · 2023-04-27T09:48:18Z

Rebased main again and removed the file ownership which seems to not work.

vincentfree · 2023-04-27T20:23:57Z

All should be good now

MovieStoreGuy

Could I ask you to update the following:

Load definition from a provided configuration
Provide a means of creating immutable Avro serialises

I really worry that it will over complicate the required logic to make it both concurrency safe and feel more at ease with static serialisers.

It is my understanding that Avro will need definition so it knows how to translate the values, hence the Init method?

I think it makes sense to update the marshaler's loaders to include reading from *Config.Encoding (I forget the actual type, but something to that affect).

I have a few other comments elsewhere but it is more to improve my understand and provide more help.

receiver/kafkareceiver/avro_unmarshaler.go

MovieStoreGuy · 2023-05-08T12:14:16Z

receiver/kafkareceiver/avro_unmarshaler.go

+}
+
+func newAVROFileSchemaDeserializer(path string) (avroDeserializer, error) {
+	schema, err := loadAVROSchemaFromFile(path)


If possible, could we avoid having a strict method to read from disk when ideally we'd want to read the definition from the configuration?

If you want to make it easier to test with, I changing the method handler to:

func readAVROSchema(r io.Reader) (schema, error)

This would allow you to use bytes.NewBuffer, strings.Reader, or os.File without needing to change the code path that does the main work and make it easier to test.

Another reason we'd want to avoid it to require loading from disk is that we also have to considering the potential security vector that someone could try load from incorrect path (ie: ../../../../../../../../../etc/passwd).

The config loaders can do this for us to make life easier and not having to repeat the same work again*

*Unless we really must have to.

Thanks for your comments, i'll address them.

github-actions · 2023-05-26T22:03:54Z

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

spockz · 2023-07-12T09:13:57Z

Who needs to sign off on this PR?

atoulme · 2023-07-12T21:13:00Z

@MovieStoreGuy is the right person to review. I have also triggered the tests just now and removed the stale tag. Please follow up if tests fail.

github-actions · 2023-07-29T05:19:17Z

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

vincentfree · 2023-07-29T06:25:39Z

@MovieStoreGu'm can you or someone else please review this PR?

@atoulme @pavolloffay Is there a larger group of maintainers that could review this PR?

MovieStoreGuy

The changes make sense to me but I would like to see a exporter definition in a future PR please.

MovieStoreGuy · 2023-08-09T15:15:14Z

receiver/kafkareceiver/avro_unmarshaler.go

+
+func setAttribute(attributes *pcommon.Map, key string, value interface{}) {
+	attribute := attributes.PutEmpty(key)
+	_ = attribute.FromRaw(value)


Is this the error being ignored?

Sorry, this should have read, Why is this error be ignored?

MovieStoreGuy · 2023-08-09T15:18:37Z

receiver/kafkareceiver/README.md

+  kafka:
+    encoding: avro
+    avro:
+      schema: |


Is there a reference to an external resource that would help someone who is new to avro to understand what is best to put here?

MovieStoreGuy · 2023-08-09T15:22:40Z

Apologies that I hadn't reviewed this sooner.

github-actions · 2023-09-01T05:19:21Z

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

github-actions · 2023-10-04T05:19:11Z

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

github-actions · 2023-10-19T05:19:00Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

thmshmm · 2023-10-25T07:09:08Z

please reopen, pushed the reimplementation based on my comments from #21067

vincentfree requested a review from a team April 19, 2023 05:58

vincentfree requested a review from MovieStoreGuy as a code owner April 19, 2023 05:58

github-actions bot assigned TylerHelmuth Apr 19, 2023

github-actions bot added the receiver/kafka label Apr 19, 2023

github-actions bot requested a review from pavolloffay April 19, 2023 05:58

atoulme approved these changes Apr 20, 2023

View reviewed changes

thmshmm force-pushed the feature/kafkareceiver-avro branch 3 times, most recently from 0ce432e to 39902ad Compare April 24, 2023 07:18

vincentfree force-pushed the feature/kafkareceiver-avro branch from 9155c14 to c27c617 Compare April 24, 2023 07:52

thmshmm force-pushed the feature/kafkareceiver-avro branch from c27c617 to 39902ad Compare April 24, 2023 08:00

vincentfree force-pushed the feature/kafkareceiver-avro branch from 39902ad to b5ceb8e Compare April 24, 2023 08:05

thmshmm force-pushed the feature/kafkareceiver-avro branch from c3225ea to 52a1ba8 Compare April 27, 2023 07:37

vincentfree force-pushed the feature/kafkareceiver-avro branch from 02d9e00 to a925d08 Compare April 27, 2023 20:19

atoulme approved these changes Apr 27, 2023

View reviewed changes

thmshmm force-pushed the feature/kafkareceiver-avro branch from a925d08 to b55de84 Compare May 8, 2023 11:47

MovieStoreGuy requested changes May 8, 2023

View reviewed changes

thmshmm force-pushed the feature/kafkareceiver-avro branch from b55de84 to 4fa0447 Compare May 8, 2023 12:44

github-actions bot added the Stale label May 26, 2023

thmshmm force-pushed the feature/kafkareceiver-avro branch from 4fa0447 to 9a8a506 Compare June 7, 2023 09:06

atoulme removed the Stale label Jul 12, 2023

thmshmm and others added 3 commits July 14, 2023 09:33

[receiver/kafkareceiver] Add AVRO encoding support

f7aef95

[receiver/kafkareceiver] update config and chloggen

9cca220

[receiver/kafkareceiver] Load avro schema from config

250a3d4

thmshmm force-pushed the feature/kafkareceiver-avro branch from 5cc6a5d to 250a3d4 Compare July 14, 2023 07:39

github-actions bot added the Stale label Jul 29, 2023

github-actions bot removed the Stale label Jul 30, 2023

MovieStoreGuy reviewed Aug 9, 2023

View reviewed changes

vincentfree added 3 commits August 9, 2023 21:25

update Readme with a reference to the avro spec

f058d63

add err handling with cleanup

47b5a99

Merge branch 'main' into feature/kafkareceiver-avro

0ce8035

vincentfree requested a review from MovieStoreGuy August 12, 2023 14:05

update dependencies

112b272

github-actions bot added the Stale label Sep 1, 2023

Merge branch 'main' into feature/kafkareceiver-avro

f01316e

github-actions bot removed the Stale label Sep 8, 2023

Merge branch 'open-telemetry:main' into feature/kafkareceiver-avro

0388f54

github-actions bot added the Stale label Oct 4, 2023

github-actions bot closed this Oct 19, 2023

thmshmm mentioned this pull request Nov 6, 2023

[receiver/kafkareceiver] Add AVRO encoding support #28897

Closed

vincentfree mentioned this pull request Mar 21, 2024

REQUEST: New membership for @vincentfree open-telemetry/community#2013

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/kafkareceiver] Feature/kafkareceiver avro #21068

[receiver/kafkareceiver] Feature/kafkareceiver avro #21068

vincentfree commented Apr 19, 2023

linux-foundation-easycla bot commented Apr 19, 2023 •

edited

Loading

atoulme left a comment

thmshmm commented Apr 21, 2023

atoulme commented Apr 21, 2023

thmshmm commented Apr 21, 2023

atoulme commented Apr 22, 2023

thmshmm commented Apr 24, 2023

atoulme commented Apr 24, 2023

thmshmm commented Apr 27, 2023

vincentfree commented Apr 27, 2023

MovieStoreGuy left a comment

MovieStoreGuy May 8, 2023

MovieStoreGuy May 8, 2023

thmshmm May 8, 2023

github-actions bot commented May 26, 2023

spockz commented Jul 12, 2023

atoulme commented Jul 12, 2023

github-actions bot commented Jul 29, 2023

vincentfree commented Jul 29, 2023 •

edited

Loading

MovieStoreGuy left a comment

MovieStoreGuy Aug 9, 2023

MovieStoreGuy Aug 17, 2023

MovieStoreGuy Aug 9, 2023

MovieStoreGuy commented Aug 9, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 4, 2023

github-actions bot commented Oct 19, 2023

thmshmm commented Oct 25, 2023

[receiver/kafkareceiver] Feature/kafkareceiver avro #21068

[receiver/kafkareceiver] Feature/kafkareceiver avro #21068

Conversation

vincentfree commented Apr 19, 2023

linux-foundation-easycla bot commented Apr 19, 2023 • edited Loading

atoulme left a comment

Choose a reason for hiding this comment

thmshmm commented Apr 21, 2023

atoulme commented Apr 21, 2023

thmshmm commented Apr 21, 2023

atoulme commented Apr 22, 2023

thmshmm commented Apr 24, 2023

atoulme commented Apr 24, 2023

thmshmm commented Apr 27, 2023

vincentfree commented Apr 27, 2023

MovieStoreGuy left a comment

Choose a reason for hiding this comment

MovieStoreGuy May 8, 2023

Choose a reason for hiding this comment

MovieStoreGuy May 8, 2023

Choose a reason for hiding this comment

thmshmm May 8, 2023

Choose a reason for hiding this comment

github-actions bot commented May 26, 2023

spockz commented Jul 12, 2023

atoulme commented Jul 12, 2023

github-actions bot commented Jul 29, 2023

vincentfree commented Jul 29, 2023 • edited Loading

MovieStoreGuy left a comment

Choose a reason for hiding this comment

MovieStoreGuy Aug 9, 2023

Choose a reason for hiding this comment

MovieStoreGuy Aug 17, 2023

Choose a reason for hiding this comment

MovieStoreGuy Aug 9, 2023

Choose a reason for hiding this comment

MovieStoreGuy commented Aug 9, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 4, 2023

github-actions bot commented Oct 19, 2023

thmshmm commented Oct 25, 2023

linux-foundation-easycla bot commented Apr 19, 2023 •

edited

Loading

vincentfree commented Jul 29, 2023 •

edited

Loading