-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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/elasticsearch] Add reliability tests #31848
Conversation
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
return rCancel | ||
} | ||
|
||
func sendLogs(t *testing.T, target, uid string, logs, agents int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[For Reviewers] I wanted to use telemetrygen here instead of writing something new but it is only exposed as a command. Does it make sense to refactor telemetrygen
as a follow-up and make it a bit easier to use as a package?
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
exporter/elasticsearchexporter/integrationtests/exporter_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor suggestions. Tests themselves LGTM - it's really nice to have these in place!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of minor comments.
exporter/elasticsearchexporter/integrationtest/exporter_test.go
Outdated
Show resolved
Hide resolved
exporter/elasticsearchexporter/integrationtest/exporter_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I know you're trying to change the ES mock to count the number of bulk requests and try to change the response status based on that. If that works out without too much complexity, 👍. Otherwise, I'm OK with the current implementation.
@JaredTan95 Can you help with a review on the PR when you get the chance? |
exporter/elasticsearchexporter/integrationtest/exporter_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
koanf dep not dropped after latest commit. go mod tidy
maybe? Otherwise lgtm
@JaredTan95 @bryan-aguilar 👋 folks, any chance I could get this one reviewed? |
Also pinging @open-telemetry/collector-contrib-approvers in case someone is available to review this PR and move it forward. Thank you! |
You'll probably need to do a rebase. Some changes have been made on main to update the go versions that are causing go vuln check to fail. I don't think this would be a blocker but did you consider integrating with the Collector testbed instead? It has a lot of the scaffolding already to create a collector instance and custom data generators and validators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating the testbed. Just left a nit, otherwise LGTM once @bryan-aguilar's comments are addressed.
@bryan-aguilar @ycombinator Addressed the review comments. |
|
@bryan-aguilar Fixed all the lints I could find. Hope all should be green now, requires your approval to build the other workflows 🙏 |
Apologies for so much noise here, I have now run |
Looks like it needs another tidy
|
Some version changes were merged and the branch got outdated. I have updated with main and ran another tidy. |
Description: Adds integration tests for the elasticsearch exporter.
Link to tracking Issue: N/A
Testing: N/A
Documentation: N/A
I have not added a changelog because the contributing guidelines indicated that we might not require any for the changes in this PR.