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

impl(otel): add OTelContext infrastructure #12887

Merged
merged 5 commits into from
Oct 14, 2023

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Oct 12, 2023

Part of the work for #12880

For more context on OTelContext, Googlers should see go/cloud-cxx:otel-async-context-dd or go/cloud-cxx:otel-async-context-slides


This change is Reviewable

@dbolduc dbolduc temporarily deployed to internal October 12, 2023 21:18 — with GitHub Actions Inactive
@dbolduc dbolduc temporarily deployed to internal October 12, 2023 22:22 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

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

Comparison is base (cd6da18) 93.59% compared to head (58d0c31) 93.60%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #12887    +/-   ##
========================================
  Coverage   93.59%   93.60%            
========================================
  Files        2064     2067     +3     
  Lines      179808   179917   +109     
========================================
+ Hits       168300   168409   +109     
  Misses      11508    11508            
Files Coverage Δ
...oogle/cloud/internal/opentelemetry_context_test.cc 100.00% <100.00%> (ø)
google/cloud/internal/opentelemetry_context.cc 96.29% <96.29%> (ø)
google/cloud/internal/opentelemetry_context.h 92.30% <92.30%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbolduc dbolduc temporarily deployed to internal October 12, 2023 23:36 — with GitHub Actions Inactive
@dbolduc dbolduc marked this pull request as ready for review October 13, 2023 00:04
@dbolduc dbolduc requested a review from a team as a code owner October 13, 2023 00:04

void DetachOTelContext(opentelemetry::context::Context const& context) {
auto& v = ThreadLocalOTelContext();
auto it = std::find_if(v.begin(), v.end(), [&context](auto const& p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought... maybe we should keep these in a multiset or some other data structure where std::find() can run faster?

Even a std::list would be better as the .erase() for vector is $O(n)$, whereas std::list::erase is $O(1)$. You still would have a $O(n)$ search.. that is where using std::multiset<> would give you $O(ln(n))$

Copy link
Member Author

Choose a reason for hiding this comment

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

We would only see duplicated Context objects in the list in practice when the futures are executed immediately. In that case we would basically take the current stack and duplicate it.

I think this was a tad lazy, so now we will check if we are in the same thread, and if so, we will not touch the stack.


Re: time complexity....

Now that the Context objects in the stack are all unique, and we can only compare equal, we cannot beat $O(n)$ search. (Also, I will remind that n <= 5).

Updated to use a std::list for the better erase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see v.erase(it, v.end()), which seems to be O(n) no matter what container you use.

Copy link
Member

@alevenberg alevenberg Oct 13, 2023

Choose a reason for hiding this comment

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

If you are erasing from the back, do you need to use find anyway?

This is what I thought when I first looked at the doc

deque stack;

while(stack.back() != c) {
stack.pop_back();
}

(no need to make changes, just thought I'd leave my thoughts)

Updated: saw convo in chat nevermind

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are erasing from the back, do you need to use find anyway?

This is what I thought when I first looked at the doc

deque stack;

while(stack.back() != c) {
stack.pop_back();
}

Yeah, that is true, and how I was thinking about it. Your API might be better.

The OTel Detach() implementation will find the element in their stack (searching from the back) and pop every element along the way (assuming the element is in the stack). I was copying that, but it was probably over cautious.

I think we can assume that the thing we are trying to pop is either back(), or it isn't found in the stack. I don't think we would leak memory.

Copy link
Member

Choose a reason for hiding this comment

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

I think your impl is good. What if it doesn't find it on the stack? My suggestion would delete the whole stack anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think it is either back() or not there the code would be simpler (don't forget the empty collection case).

Otherwise, consider std::find_if(stack.rbegin(), stack.rend(), ...);

@dbolduc dbolduc force-pushed the otel-context-setup branch from 8fc0e0f to 8324cdc Compare October 13, 2023 16:13
@dbolduc dbolduc temporarily deployed to internal October 13, 2023 16:13 — with GitHub Actions Inactive
@dbolduc dbolduc temporarily deployed to internal October 13, 2023 21:29 — with GitHub Actions Inactive
@@ -70,8 +81,8 @@ class ScopedOTelContext {

~ScopedOTelContext() {
if (same_thread_) return;
for (auto const& c : contexts_) {
DetachOTelContext(c);
for (auto it = contexts_.rbegin(); it != contexts_.rend(); ++it) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is some kind of "Remove Common Suffix", something like:

~ScopedOTelContext() {
...
  DetachOTelMany(std::move(context_));
}

void DetachOTelMany(std::vector<opentelemetry::context::Context> c) {
  auto& v = ThreadLocalOTelContext();
  auto p = std::mismatch(c.rbegin(), c.rend(), v.rbegin(), r.end(), [](auto const& a, auto const& b) {
    return a == b.first;
  });
  std::erase(v.rend(), p.second);
}

https://en.cppreference.com/w/cpp/algorithm/mismatch

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL: std::mismatch

Seems like this is some kind of "Remove Common Suffix"

That is what I am trying to do. But I am struggling to erase using the reverse iterators.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://godbolt.org/z/raG9K4ocW though I think it does not matter for small collections.

@dbolduc dbolduc merged commit 88ab0a4 into googleapis:main Oct 14, 2023
@dbolduc dbolduc deleted the otel-context-setup branch October 14, 2023 12:01
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.

4 participants