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

[chore] add test helper to verify exporter behavior on errors #8143

Merged
merged 14 commits into from
Oct 24, 2023

Conversation

omrozowicz-splunk
Copy link
Contributor

@omrozowicz-splunk omrozowicz-splunk commented Jul 28, 2023

Description: Adding a test helper to test exporter behaviour on errors.

Link to tracking Issue: #7479
Will also fix: #7481

It is created based on the exporter receiver test: #7516
The exporter targets mockReceiver as an endpoint.

The test cases covered here are:

  1. ConsumeLogs/Traces/Metrics call succeeds at the first try
  2. ConsumeLogs/Traces/Metrics call fails once with non-permanent error, then succeeds on one of the next tries
  3. ConsumeLogs/Traces/Metrics call fails once with permanent error and the data is not retried
  4. ConsumeLogs/Traces/Metrics call fails once with permanent error or permanent error (then the test pass requirement is calculated based on number of errors per kind/successfully delivered data/total number of data to be delivered)

@omrozowicz-splunk omrozowicz-splunk changed the title [WIP] add test helper to verify exporter behavior on errors [WIP] [chore] add test helper to verify exporter behavior on errors Aug 1, 2023
@omrozowicz-splunk omrozowicz-splunk changed the title [WIP] [chore] add test helper to verify exporter behavior on errors [chore] add test helper to verify exporter behavior on errors Aug 1, 2023
@omrozowicz-splunk omrozowicz-splunk marked this pull request as ready for review August 1, 2023 13:55
@omrozowicz-splunk omrozowicz-splunk requested review from a team and bogdandrutu August 1, 2023 13:55
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Files Coverage Δ
exporter/exportertest/mock_consumer.go 100.00% <100.00%> (ø)
exporter/exportertest/contract_checker.go 98.90% <98.90%> (ø)

... and 3 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

connector/forwardconnector/go.mod Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/contract_checker.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/otlpexporter/consume_contract_test.go Outdated Show resolved Hide resolved
exporter/otlpexporter/consume_contract_test.go Outdated Show resolved Hide resolved
exporter/otlpexporter/consume_contract_test.go Outdated Show resolved Hide resolved
exporter/otlpexporter/mock_receiver.go Outdated Show resolved Hide resolved
exporter/otlpexporter/mock_receiver.go Outdated Show resolved Hide resolved
exporter/otlpexporter/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/contract_checker.go Outdated Show resolved Hide resolved
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Also, please rebase

exporter/otlpexporter/consume_contract_test.go Outdated Show resolved Hide resolved
exporter/exportertest/contract_checker.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
@omrozowicz-splunk
Copy link
Contributor Author

Also, please rebase

done

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

I think we're almost done :) Thank you, @omrozowicz-splunk

exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/go.mod Show resolved Hide resolved
receiver/otlpreceiver/go.mod Show resolved Hide resolved
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

@omrozowicz-splunk couple last comments. Please rebase and I think we can merge it. Thank you!

exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
exporter/exportertest/mock_receiver.go Outdated Show resolved Hide resolved
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @omrozowicz-splunk!

@dmitryax
Copy link
Member

dmitryax commented Oct 18, 2023

@omrozowicz-splunk one last thing. Can you please add contract_checker_test.go with some basic test coverage. Looks like usage from another module isn't counted and it contract_checker_test.go the project's coverage below the threshold. Similar to receivertest/contract_checker_test.go which should showcase how to use the helper for other exporters

@omrozowicz-splunk
Copy link
Contributor Author

@omrozowicz-splunk one last thing. Can you please add contract_checker_test.go with some basic test coverage. Looks like usage from another module isn't counted and it contract_checker_test.go the project's coverage below the threshold. Similar to receivertest/contract_checker_test.go which should showcase how to use the helper for other exporters

Okaay, I can try to do that, but that will prolong this task even more - it requires a fake receiver and fake exporter and making the fake exporter send data to that fake receiver. Not sure if that will be super helpful, probably looking at the otlp example will be easier to understand.
But I'll take a look, maybe it is easier than I imagine now 😅

@dmitryax
Copy link
Member

Unfortunately, if we merge this without the tests, the collector's build will marked as failing because the project's coverage will be below the threshold

@omrozowicz-splunk
Copy link
Contributor Author

Hey @dmitryax, I added contract_checker_test.go, it's probably not the most elegant, but works and will increase the coverage. I needed to change requestCounter in MockConsumer struct to *requestCounter to be able to control it from exporter and then passing over to receiver. Please take a look 😄

@dmitryax dmitryax force-pushed the add-exporter-test branch 3 times, most recently from 902ef99 to 2ead251 Compare October 23, 2023 23:50
- Remove redundant exposed API
- Simplify contract_checker_test.go
- Remove excessive logging
- Pass correct testing.T to subtests
@dmitryax dmitryax merged commit e97ceca into open-telemetry:main Oct 24, 2023
31 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 24, 2023
@dmitryax
Copy link
Member

Thanks @omrozowicz-splunk! I added another commit to reduce the exposed API more and to simplify some tests. Let me know if something doesn’t look right for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[otlpexporter] Add test to verify behavior on destination errors
2 participants