-
Notifications
You must be signed in to change notification settings - Fork 453
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
Closed
cijothomas
wants to merge
48
commits into
open-telemetry:main
from
cijothomas:cijothomas/periodicreader
Closed
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
e5678e8
Use dedicated thread for metrics processing
cijothomas f2eff31
Merge branch 'main' into cijothomas/periodicreader
cijothomas 91f7c2e
add comment on shutdown
cijothomas aedbd2c
pass timeout to exporters
cijothomas 4198792
named thread and pass timeout to export
cijothomas 966c9a6
test fixes
cijothomas 8f71c7b
Add tests for periodic reader from various RT combinations (#2147)
cijothomas 63728cc
Update doc comments (#2149)
utpilla c85ae55
Use weaver for semantic convention codegen (#2098)
lquerel d2c4ef5
Fix CI - Regenerate proto files, and selective msrv-patching (#2159)
lalitb 6be205b
For Delta Temporarlity, avoid exporting when no new measurements are …
cijothomas 7182be1
Improve internal opentelemetry logging - directly using tracing mcros…
lalitb 5003dd3
MeterProvider modified to not do shutdown if user has already shut do…
cijothomas 29ec004
Minor cleanups in Metrics module (#2155)
cijothomas ea2b9a7
Prepare 0.26.0 release, update Metrics API to Beta (#2160)
cijothomas ea0d788
Nit fixes to bug template (#2161)
cijothomas b96ab43
Metrics - Instrument Name Validation fixes (#2166)
cijothomas 9eaba05
Improve internal opentelemetry logging (#2128)
lalitb e1e993d
Remove comments that are not relevant anymore (#2171)
cijothomas f17cde9
Docs: Remove unnecessary indentation for better readability (#2174)
moshensky 10e2df8
Merge branch 'main' into cijothomas/periodicreader
cijothomas 2b562b6
bring new change back
cijothomas 020fb03
add internal logs liberally for now
cijothomas 018cfde
fix otlp exporter for metric to not need rt
cijothomas a5b7de2
fix fmt
cijothomas 58a3ba1
clenaups
cijothomas 30b09a4
let more sleep for CI
cijothomas e51bca6
tests from tokio rt
cijothomas bc2aea2
test refactor
cijothomas 218d9bb
make sync
cijothomas b78a3f3
small fix
cijothomas 76899f2
fix stdout
cijothomas 9e3be6a
inmeory
cijothomas ec4384d
continue on error for ci
cijothomas 1e98f8c
remove sync
cijothomas 75b4006
reomve ynsc
cijothomas 84fb885
use mutex for messag
cijothomas 3a12310
test for exporter failures
cijothomas b231d19
Merge branch 'main' into cijothomas/periodicreader
cijothomas 6cc1428
fix clippy
cijothomas 75fecfe
add async callbacks test
cijothomas 883b337
fix clippy
cijothomas 5a5eea4
reveiw comment
cijothomas 9181b09
Merge branch 'main' into cijothomas/periodicreader
cijothomas fb26138
log interval in milliseconds
cijothomas f8b6c74
Merge branch 'main' into cijothomas/periodicreader
cijothomas c3f6931
Merge branch 'main' into cijothomas/periodicreader
cijothomas e0d7126
Merge branch 'main' into cijothomas/periodicreader
cijothomas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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 ?