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

[receiver/elasticsearchreceiver] Add elasticsearch integration test #10361

Merged
merged 13 commits into from
Jun 9, 2022

Conversation

Frapschen
Copy link
Contributor

Description:
Add elasticsearch integration test.

@Frapschen Frapschen requested a review from a team May 26, 2022 15:34
@Frapschen Frapschen requested a review from djaglowski as a code owner May 26, 2022 15:34
@Frapschen
Copy link
Contributor Author

#10165

require.NoError(t, err)

scrapertest.CompareMetrics(expectedMetrics, actualMtrics, scrapertest.IgnoreMetricValues())
//err = scrapertest.CompareMetrics(expectedMetrics, actualMtrics, scrapertest.IgnoreMetricValues())
Copy link
Member

Choose a reason for hiding this comment

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

Were you able to run this receiver locally against the container? (Is that how you generated the result?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I run this reveiver locally and get the result. I have checked the es container by call http://localhost:9200,its seem running well.

Copy link
Member

Choose a reason for hiding this comment

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

If it's working for you locally, then I suspect this is an issue with not using a "wait strategy" to verify that the application is ready. Without a wait strategy, the container may start, but the application within it may not be ready to respond by the time the scraper tries to query it.

Here's an example of a typical wait strategy.

Here's an example of a custom wait strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx ,I will have a try...

require.NoError(t, err)

scrapertest.CompareMetrics(expectedMetrics, actualMtrics, scrapertest.IgnoreMetricValues())
//err = scrapertest.CompareMetrics(expectedMetrics, actualMtrics, scrapertest.IgnoreMetricValues())
Copy link
Member

Choose a reason for hiding this comment

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

If it's working for you locally, then I suspect this is an issue with not using a "wait strategy" to verify that the application is ready. Without a wait strategy, the container may start, but the application within it may not be ready to respond by the time the scraper tries to query it.

Here's an example of a typical wait strategy.

Here's an example of a custom wait strategy.

Co-authored-by: Daniel Jaglowski <[email protected]>

f := NewFactory()
cfg := f.CreateDefaultConfig().(*Config)
cfg.Endpoint = net.JoinHostPort(hostname, "9200")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure out what is going on, this line set cfg.Endpoint="localhost:9200", actullay, we need http://localhost:9200.

@Frapschen
Copy link
Contributor Author

hi, after that problem, I scraped the data from es container, and the CompareMetrics() get two errors in it return

missing expected resource with attributes: map[elasticsearch.cluster.name:docker-cluster elasticsearch.node.name:917e13e55eed]

extra resource with attributes: map[elasticsearch.cluster.name:docker-cluster elasticsearch.node.name:db6ca60ce36f]

I think, its probably fine for the test. pls review :)

@Frapschen
Copy link
Contributor Author

@djaglowski

@djaglowski
Copy link
Member

@Frapschen A similar issue has prompted #10129.

For this immediate problem, I am ok with the fact that the resource attribute is unpredictable. However, the test will of course need to pass. One way to make this work immediately would be to override the attribute in either the expected or actual resource, or both.

@Frapschen
Copy link
Contributor Author

Frapschen commented Jun 7, 2022

One way to make this work immediately would be to override the attribute in either the expected or actual resource, or both.

@djaglowski I can override the expected resource, but I don't know that can achieve the test purpose, or we just write the actual resource and ignore the err?

@djaglowski
Copy link
Member

@Frapschen, I think in this case it is ok to overwrite the randomly generated value of elasticsearch.node.name. We cannot predict this attribute value, but we can validate the rest of the result. This is a similar concept to how we ignore the metric values, since they are often not predictable in a live environment.

@Frapschen
Copy link
Contributor Author

@djaglowski I override the elasticsearch.node.name with the expected one, none err occur.

@djaglowski
Copy link
Member

@Frapschen, #10828 was just merged, which should provide you with a CompareOption to ignore the attribute value (rather than having to overwrite it). Will you please switch to using that?

Frapschen added 2 commits June 9, 2022 11:43
# Conflicts:
#	CHANGELOG.md
#	receiver/elasticsearchreceiver/go.sum
@Frapschen
Copy link
Contributor Author

@djaglowski used IgnoreResourceAttributeValue and get none error.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Thank you @Frapschen!

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Looks like we need to figure out what's going on with this test on windows:

=== CONT  TestElasticsearchIntegration/Running_elasticsearch_7.9
    integration_test.go:91: 
        	Error Trace:	integration_test.go:91
        	            				integration_test.go:50
        	Error:      	Received unexpected error:
        	            	Error response from daemon: could not find plugin bridge in v1 plugin registry: plugin not found: failed to create container
        	Test:       	TestElasticsearchIntegration/Running_elasticsearch_7.9
--- FAIL: TestElasticsearchIntegration (0.00s)
    --- FAIL: TestElasticsearchIntegration/Running_elasticsearch_7.9 (15.12s)

@Frapschen Frapschen requested a review from djaglowski June 9, 2022 14:59
@djaglowski djaglowski merged commit 333e9a5 into open-telemetry:main Jun 9, 2022
kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
…pen-telemetry#10361)

* init elasticsearch integration test.

* Add elasticsearch integration test.

* Apply suggestions from code review

Co-authored-by: Daniel Jaglowski <[email protected]>

* resolve endpoint miss schema

* resolve conflict.

* update changelog

* save

* update

* override the attribute to the expected one

* apply reviewer's suggestion.

* Update integration_test.go

Co-authored-by: Daniel Jaglowski <[email protected]>
@Frapschen Frapschen deleted the add_es_integration_test branch July 7, 2022 05:49
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.

3 participants