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.

Sign up for GitHub

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

Jump to bottom

[receiver/kafkareceiver] Add encoding extensions support #33888

Merged
merged 1 commit into from
Sep 9, 2024
Merged

[receiver/kafkareceiver] Add encoding extensions support #33888

merged 1 commit into from
Sep 9, 2024

Conversation

thmshmm
Copy link
Contributor

@thmshmm thmshmm commented Jul 3, 2024 •
edited
Loading

Description: Add support for encoding extensions in the kafkareceiver

To be able to use encoding extensions this PR adds extension support and proposes to rename the existing encoding configuration property to format and reusing the encoding property for configuring encoding extensions. Reason is to be consistent with other receivers/exporters like the fileexporter that already support extensions.

Link to tracking Issue: n/a

Testing: Tested with the existing avro_log_encoding extension as well with receivers internal json encoding.

Documentation:: Updated README.md within the receiver describing the use of encoding extensions.

@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from b29513a to c38502d Compare July 10, 2024 15:15
@thmshmm thmshmm marked this pull request as ready for review July 10, 2024 15:18
@thmshmm thmshmm requested a review from a team July 10, 2024 15:18
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from c38502d to 9b9cb8f Compare July 11, 2024 19:01
atoulme
atoulme approved these changes Jul 20, 2024
View reviewed changes
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

it's a breaking change - any way to smooth that over? But it looks good.

@thmshmm
Copy link
Contributor Author

thmshmm commented Jul 21, 2024

it can be done, keeping the encoding config property and making it type any, then checking if the internal unmarshalers or a encoding extension should be used. but due to the factory pre-creating all unmarshallers and the encoding is loaded later in the Start that part needs to be rewritten as well.

another alternative would be to keep the existing encoding config name for the internal unmarshallers and introduce a new one for the encoding extension support. then its non breaking as well.

@atoulme
Copy link
Contributor

atoulme commented Aug 1, 2024

Please fix the conflicts and let's go

@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from 9b9cb8f to d2a0d10 Compare August 1, 2024 11:54
@thmshmm
Copy link
Contributor Author

thmshmm commented Aug 1, 2024

Please fix the conflicts and let's go

@atoulme done. tested again with and without encoding extension and the internal encoders.

@atoulme
Copy link
Contributor

atoulme commented Aug 1, 2024

Thanks, please look at the build

@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from d2a0d10 to c0a66e7 Compare August 1, 2024 15:12
@thmshmm thmshmm mentioned this pull request Aug 1, 2024
Merged
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from c0a66e7 to a6ec1d7 Compare August 10, 2024 15:06
@thmshmm
Copy link
Contributor Author

thmshmm commented Aug 10, 2024

@MovieStoreGuy can u please review

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Aug 25, 2024
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from a6ec1d7 to 356aab1 Compare September 4, 2024 06:47
@github-actions github-actions bot removed the Stale label Sep 5, 2024
MovieStoreGuy
MovieStoreGuy reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Couple of suggestions and be happy to merge

receiver/kafkareceiver/kafka_receiver.go Outdated Show resolved Hide resolved
receiver/kafkareceiver/kafka_receiver.go Outdated Show resolved Hide resolved
receiver/kafkareceiver/kafka_receiver.go Outdated Show resolved Hide resolved
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from 57e8cdb to 880a09b Compare September 6, 2024 06:38
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from 880a09b to e1ece09 Compare September 8, 2024 15:06
@thmshmm
Copy link
Contributor Author

thmshmm commented Sep 8, 2024

as discussed. refactored so its now non breaking change. tested with the avro encoding extension as well with the internal json and text encoding.

@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from f2dda2b to 5b6e66c Compare September 9, 2024 06:29
@thmshmm thmshmm force-pushed the feature/kafkareceiver-enc-ext branch from 5b6e66c to db75618 Compare September 9, 2024 06:44
@MovieStoreGuy MovieStoreGuy merged commit cc5889d into open-telemetry:main Sep 9, 2024
155 of 156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 9, 2024
@thmshmm thmshmm deleted the feature/kafkareceiver-enc-ext branch September 9, 2024 08:50
MovieStoreGuy added a commit that referenced this pull request Sep 12, 2024
@thmshmm @MovieStoreGuy
[exporter/kafkaexporter] Add encoding extensions support (#34384)
21208c0 
**Description:** Add support for encoding extensions in the
kafkaexporter

To be able to use encoding extensions this PR adds extension support and
proposes to rename the existing `encoding` configuration property to
`format` and reusing the `encoding` property for configuring encoding
extensions. Reason is to be consistent with other receivers/exporters.

Related to
#33888
which adds encoding extension support in the `kafkareceiver`.

**Link to tracking Issue:** n/a

**Testing:** Tested via the following configuration.
```
receivers:
  kafka:
    brokers:
    - localhost:29092
    encoding: json
    group_id: test1
    topic: logs_in

extensions:
  json_log_encoding:

exporters:
  debug:
    verbosity: detailed
  kafka:
    brokers:
      - localhost:29092
    encoding: json_log_encoding
    topic: json_out

processors:
  batch:

service:
  extensions: [json_log_encoding]
  pipelines:
    logs:
      receivers: [kafka]
      processors: [batch]
      exporters: [debug, kafka]
  telemetry:
    logs:
      level: "info"
```

Any json can be written to the `logs_in` topic and results be viewed in
the `json_out` topic.

When removing `encoding: json_log_encoding` the default format type is
used and the output in `json_out` topic changes accordingly.

**Documentation:** Updated README.md within the receiver describing the
use of encoding extensions.

Co-authored-by: Sean Marciniak <30928402+MovieStoreGuy@users.noreply.github.com>
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
@thmshmm @f7o
[receiver/kafkareceiver] Add encoding extensions support (open-teleme…
2fb0d8a 
…try#33888)

**Description:** Add support for encoding extensions in the
kafkareceiver
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.-->
To be able to use encoding extensions this PR adds extension support and
proposes to rename the existing `encoding` configuration property to
`format` and reusing the `encoding` property for configuring encoding
extensions. Reason is to be consistent with other receivers/exporters
like the `fileexporter` that already support extensions.

**Link to tracking Issue:** n/a

**Testing:** Tested with the existing avro_log_encoding extension as
well with receivers internal json encoding.

**Documentation:**: Updated README.md within the receiver describing the
use of encoding extensions.
@thmshmm thmshmm mentioned this pull request Sep 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoulme atoulme atoulme approved these changes

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

Assignees

@dmitryax dmitryax

Labels
receiver/kafka
Projects
None yet
Milestone
v0.109.0
Development

Successfully merging this pull request may close these issues.
4 participants
@thmshmm @atoulme @MovieStoreGuy @dmitryax