-
Notifications
You must be signed in to change notification settings - Fork 373
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
fix(otel): reconnect async traces (e.g. LROs) #13147
fix(otel): reconnect async traces (e.g. LROs) #13147
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13147 +/- ##
==========================================
- Coverage 93.00% 92.99% -0.01%
==========================================
Files 2137 2137
Lines 185877 185898 +21
==========================================
+ Hits 172868 172884 +16
- Misses 13009 13014 +5 ☔ View full report in Codecov by Sentry. |
Ugh, the gcc-7-3 error is out of our control. I need to think about how we can avoid it in our repo. The following code reproduces the same error. It does not use any TEST(OpenTelemetry, DarrenOTelOnly) {
auto span_catcher = InstallSpanCatcher();
auto provider = opentelemetry::trace::Provider::GetTracerProvider();
auto tracer = provider->GetTracer("gcc-7-3 test");
auto parent = tracer->StartSpan("parent");
opentelemetry::trace::Scope parent_scope(parent);
auto child = tracer->StartSpan("child");
child->End();
parent->End();
EXPECT_THAT(
span_catcher->GetSpans(),
ElementsAre(AllOf(SpanNamed("child"),
SpanWithParentSpanId(parent->GetContext().span_id())),
SpanNamed("parent")));
}
|
3db9441
to
8f99a85
Compare
@@ -208,6 +210,52 @@ TEST(OpenTelemetry, TracedAsyncBackoffDisabled) { | |||
EXPECT_THAT(spans, IsEmpty()); | |||
} | |||
|
|||
TEST(OpenTelemetry, TracedAsyncBackoffPreservesContext) { | |||
if (CompilerId() == "GNU" && CompilerVersion() == "7.3.1") { |
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.
Does OTel claim to support GCC 7.x? Is there a bug in OTel we can reference? Can we detect this with an OTel version?
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.
Does OTel claim to support GCC 7.x?
My reading is they support compilers that implement the standard correctly.
Is there a bug in OTel we can reference?
good call. yes: open-telemetry/opentelemetry-cpp#1014
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.
There is a patch linked in the issue. I will try to apply it to our centos-7
dockerfile.
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.
According to some comments on that bug, GCC 8.x also trouble. You may need to apply that patch to several other builds.
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.
My reading is they support compilers that implement the standard correctly.
There are no compilers without defects. That implies they support no compilers at all.
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.
Patched.
According to some comments on that bug, GCC 8.x also trouble. You may need to apply that patch to several other builds.
Thanks for pointing that out. I forgot that the demo-install
builds do not run these unit tests. I assumed they all worked.
Opened #13159 because I am not sure if we should apply the patch by default, or just call out the issue in a comment.
There are no compilers without defects. That implies they support no compilers at all.
I realize that and my comment was tongue-in-cheek. Here is their actual text: https://github.com/open-telemetry/opentelemetry-cpp/blob/cb603ad97f33e52340e627e1cb43ba73bb1d7ef0/README.md?plain=1#L48-L49
auto span = MakeSpan("Async Backoff"); | ||
OTelScope scope(span); | ||
auto timer = cq.MakeRelativeTimer(duration); | ||
return EndSpan(span, std::move(timer)); |
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.
std::move(span)
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.
Done
b4cff02
to
1a577cb
Compare
Ping. (I am not blocked on this PR. It would just be nice to have the fix in.) |
Fixes #13141
See the linked issue for the problem write up. The changes in this PR generated the seemingly correct, second screenshot.
The ABI update was necessary because we stop defining some templated
future<StatusOr<std::chrono::...>>
type, which triggered a false positive incheck-api
.Something is buggy with (at least)
opentelemetry-cpp
+gcc 7.3.1
. So patch the Dockerfile used in that build to work around it.This change is