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.

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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

bgranetzke
Copy link
Contributor

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.

@bgranetzke
Copy link
Contributor Author

@MovieStoreGuy Sorry for creating a new PR. This should have all your recommendations incorporated. It also leverages your latest factory_test approach.

@bgranetzke
Copy link
Contributor Author

@MovieStoreGuy It was a bit more involved than initially thought. I've removed the log-based assertion of the expected marshaler being selected and replaced with an approach that validates the marshaler by assessing the exporter output.
I'd love to get your feedback. Callouts:

  1. re-introduced the ProducerFactoryFunc from the original PR. I believe the existing sarama.SyncProducer interface (along with the equivalent sarama/mocks.SyncProducer) is sufficient for testing without introducing a new abstraction.
  2. addressed your original defaulting concern (vs. defaulting to nil)
  3. Adapted your test cases to include producerFunc in the test scenario structs

@MovieStoreGuy
Copy link
Contributor

Let me have a further look to see how i can help :)

@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 Dec 20, 2022
@MovieStoreGuy
Copy link
Contributor

Once you get a chance @bgranetzke , please rebase :)

@github-actions github-actions bot removed the Stale label Dec 28, 2022
@bgranetzke
Copy link
Contributor Author

@MovieStoreGuy Thank you for the changes. Correct me if I've missed something, but there is no longer a reason for this PR, correct? Just checking before I close it. Thanks again!

@MovieStoreGuy
Copy link
Contributor

I don't believe so @bgranetzke , you should be safe to close this out.

@bgranetzke bgranetzke closed this Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants