Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use dedicated thread for Metrics PeriodicReader #2142
base: main
Are you sure you want to change the base?
Use dedicated thread for Metrics PeriodicReader #2142
Changes from 39 commits
e5678e8
f2eff31
91f7c2e
aedbd2c
4198792
966c9a6
8f71c7b
63728cc
c85ae55
d2c4ef5
6be205b
7182be1
5003dd3
29ec004
ea2b9a7
ea0d788
b96ab43
9eaba05
e1e993d
f17cde9
10e2df8
2b562b6
020fb03
018cfde
a5b7de2
58a3ba1
30b09a4
e51bca6
bc2aea2
218d9bb
b78a3f3
76899f2
9e3be6a
ec4384d
1e98f8c
75b4006
84fb885
3a12310
b231d19
6cc1428
75fecfe
883b337
5a5eea4
9181b09
fb26138
f8b6c74
c3f6931
e0d7126
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Have you verified that this still works? I believe
reqwest
relies on Tokio. Now that the outgoing Http call throughreqwest
is being made on a background thread without Tokio (or any runtime), it might be problematic. Looking to ensure that we don't hit this issue in particular:Related to #248, #2137
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.
Excellent point!
Based on offline discussion, yes this appear to cause issues when using both
reqwest
andhyper
. These libraries seem to have a strong requirement that it cannot work unless they are inside tokio runtime.tonic
seem to work fine without issues.Will check if there are ways to work around the http library limitations.
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.
I did a quick test with Simple Log Processor + OTLP exporter with request::blocking::Client - this works without need of tokio runtime. This should also work with background thread for batch in that case. Enforcing this for background thread could be one option.
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.
Yes. But there is no blocking version for libraries like tonic, effectively limiting exporting to only support http with reqwest::blockingClient.
Will need to redesign after doing a comparison of all feasible options.
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.
This maybe incorrect, as it may have worked during shutdown only. Apologies for the confusion!
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.
Yes,
tonic
is tricky, there is no alternative either.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.
@lalitb I have another correction to make about tonic, based on more testing:
The changes in this PR works fine with tonic, as long as the main function of the app is a tokio one. Yes the tonic::export call is made from our background thread, still this works.
If the main function is not a tokio main, then the app panics at meterprovider build itself. It looks like we attempt to create a grpc channel at build(), and that fails due to lack of tokio runtime.
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.
Interesting, I was thinking the spawned thread (std::thread::spawn) doesn't run in the tokio runtime and so will not have access to tokio runtime. thanks for confirming.
Trying to summarize the scenarios:
These work:
And these doesn't work:
how about this ?
Check warning on line 46 in opentelemetry-otlp/src/metric.rs
Codecov / codecov/patch
opentelemetry-otlp/src/metric.rs#L46
Check warning on line 175 in opentelemetry-otlp/src/metric.rs
Codecov / codecov/patch
opentelemetry-otlp/src/metric.rs#L175
Check warning on line 194 in opentelemetry-otlp/src/metric.rs
Codecov / codecov/patch
opentelemetry-otlp/src/metric.rs#L194
Check warning on line 283 in opentelemetry-otlp/src/metric.rs
Codecov / codecov/patch
opentelemetry-otlp/src/metric.rs#L283
Check warning on line 767 in opentelemetry-sdk/src/metrics/mod.rs
Codecov / codecov/patch
opentelemetry-sdk/src/metrics/mod.rs#L767