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

Waiting a longer time when run unit tests of http client, which may take a long time when run under valgrind. #1171

Merged

Conversation

owent
Copy link
Member

@owent owent commented Jan 13, 2022

Signed-off-by: owentou [email protected]

Fixes #1049

Changes

Increase timeout for http client. We find 8s is not enough sometime.

https://github.com/open-telemetry/opentelemetry-cpp/runs/4793495501

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team January 13, 2022 02:28
@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #1171 (e5d9c92) into main (f380fcb) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1171   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         174      174           
  Lines        6404     6404           
=======================================
  Hits         5974     5974           
  Misses        430      430           
Impacted Files Coverage Δ
ext/test/http/curl_http_test.cc 95.17% <100.00%> (ø)

@@ -251,7 +251,7 @@ TEST_F(OtlpHttpLogExporterTestPeer, ExportJsonIntegrationTest)
report_span_id.assign(span_id_hex, sizeof(span_id_hex));
}

ASSERT_TRUE(waitForRequests(8, old_count + 1));
ASSERT_TRUE(waitForRequests(30, old_count + 1));
Copy link
Member

Choose a reason for hiding this comment

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

interesting. We started with 2, then 8, and now 30. Didn't know it's that slow with Valgrind :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I found otlp http client test under valgrind takes 6s most of the time. The //sdk/test/_metrics:metric_instrument_test take the longest time(almost 1minute). But sometime it takes more time to finish, maybe the CI node is busy at that time?

Copy link
Member

@lalitb lalitb Jan 13, 2022

Choose a reason for hiding this comment

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

Why do we need to increase the HTTP wait time as part of this PR if it's 6 secs most of the time even with valgrind? We should eventually remove the old metrics tests once new implementation is ready.

Copy link
Member

Choose a reason for hiding this comment

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

This failure is happening every now and then in my PR recently.

Copy link
Member

Choose a reason for hiding this comment

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

We can merge this PR if that's the case. If the problem persists, we can think of mocking the HTTP responses instead of using a real server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM

@lalitb
Copy link
Member

lalitb commented Jan 15, 2022

@owent - can you please rebase this PR with main, to allow merging.

@owent owent force-pushed the propagating_resource_through_logger_provider branch from 7c70e6b to e5d9c92 Compare January 16, 2022 02:04
@owent
Copy link
Member Author

owent commented Jan 16, 2022

@owent - can you please rebase this PR with main, to allow merging.

Done

@lalitb lalitb merged commit bc1b469 into open-telemetry:main Jan 16, 2022
@owent owent deleted the propagating_resource_through_logger_provider branch February 11, 2022 15:31
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.

Unit test failing in CI: OtlpHttpExporterTestPeer.ExportJsonIntegrationTest
3 participants