-
Notifications
You must be signed in to change notification settings - Fork 450
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 native Rust support for async traits in LogExporter::export() method #2374
base: main
Are you sure you want to change the base?
Changes from 12 commits
d454536
3ed482d
a2aa648
8ba1173
37823e7
3b4cd57
858cd87
e6472bf
8cacf52
dfac978
f752d50
6ac8aa0
4924e3b
8db475c
4a6e36f
3af54c3
8c34380
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,11 +247,17 @@ | |
|
||
#[async_trait] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't need async-trait anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this can be safely removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unresolving to remove async_trait |
||
impl LogExporter for ReentrantLogExporter { | ||
async fn export(&self, _batch: LogBatch<'_>) -> LogResult<()> { | ||
// This will cause a deadlock as the export itself creates a log | ||
// while still within the lock of the SimpleLogProcessor. | ||
warn!(name: "my-event-name", target: "reentrant", event_id = 20, user_name = "otel", user_email = "[email protected]"); | ||
Ok(()) | ||
#[allow(clippy::manual_async_fn)] | ||
fn export<'a>( | ||
&'a self, | ||
_batch: &'a LogBatch<'a>, | ||
) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a { | ||
async { | ||
// This will cause a deadlock as the export itself creates a log | ||
// while still within the lock of the SimpleLogProcessor. | ||
warn!(name: "my-event-name", target: "reentrant", event_id = 20, user_name = "otel", user_email = "[email protected]"); | ||
Ok(()) | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,35 +58,40 @@ | |
|
||
#[async_trait] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can remove async_trait from everywhere now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done, thanks. |
||
impl LogExporter for TonicLogsClient { | ||
async fn export(&self, batch: LogBatch<'_>) -> LogResult<()> { | ||
let (mut client, metadata, extensions) = match &self.inner { | ||
Some(inner) => { | ||
let (m, e, _) = inner | ||
.interceptor | ||
.lock() | ||
.await // tokio::sync::Mutex doesn't return a poisoned error, so we can safely use the interceptor here | ||
.call(Request::new(())) | ||
.map_err(|e| LogError::Other(Box::new(e)))? | ||
.into_parts(); | ||
(inner.client.clone(), m, e) | ||
} | ||
None => return Err(LogError::Other("exporter is already shut down".into())), | ||
}; | ||
#[allow(clippy::manual_async_fn)] | ||
fn export<'a>( | ||
&'a self, | ||
batch: &'a LogBatch<'a>, | ||
) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a { | ||
async move { | ||
let (mut client, metadata, extensions) = match &self.inner { | ||
Some(inner) => { | ||
let (m, e, _) = inner | ||
.interceptor | ||
.lock() | ||
.await // tokio::sync::Mutex doesn't return a poisoned error, so we can safely use the interceptor here | ||
.call(Request::new(())) | ||
.map_err(|e| LogError::Other(Box::new(e)))? | ||
.into_parts(); | ||
(inner.client.clone(), m, e) | ||
} | ||
None => return Err(LogError::Other("exporter is already shut down".into())), | ||
}; | ||
|
||
let resource_logs = group_logs_by_resource_and_scope(batch, &self.resource); | ||
let resource_logs = group_logs_by_resource_and_scope(batch, &self.resource); | ||
|
||
otel_debug!(name: "TonicsLogsClient.CallingExport"); | ||
otel_debug!(name: "TonicsLogsClient.CallingExport"); | ||
|
||
client | ||
.export(Request::from_parts( | ||
metadata, | ||
extensions, | ||
ExportLogsServiceRequest { resource_logs }, | ||
)) | ||
.await | ||
.map_err(crate::Error::from)?; | ||
|
||
Ok(()) | ||
client | ||
.export(Request::from_parts( | ||
metadata, | ||
extensions, | ||
ExportLogsServiceRequest { resource_logs }, | ||
)) | ||
.await | ||
.map_err(crate::Error::from)?; | ||
Ok(()) | ||
} | ||
} | ||
|
||
fn shutdown(&mut self) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,7 +108,16 @@ | |
/// OTLP exporter that sends log data | ||
#[derive(Debug)] | ||
pub struct LogExporter { | ||
client: Box<dyn opentelemetry_sdk::export::logs::LogExporter>, | ||
//client: Box<dyn opentelemetry_sdk::export::logs::LogExporter>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can remove this now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, removed. thanks |
||
client: LogExporterInner, | ||
} | ||
|
||
#[derive(Debug)] | ||
enum LogExporterInner { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name can be better IMO. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocking issue though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Have renamed it to |
||
#[cfg(feature = "grpc-tonic")] | ||
Tonic(crate::exporter::tonic::logs::TonicLogsClient), | ||
#[cfg(any(feature = "http-proto", feature = "http-json"))] | ||
Http(crate::exporter::http::OtlpHttpClient), | ||
} | ||
|
||
impl LogExporter { | ||
|
@@ -117,21 +126,44 @@ | |
LogExporterBuilder::default() | ||
} | ||
|
||
/// Create a new log exporter | ||
pub fn new(client: impl opentelemetry_sdk::export::logs::LogExporter + 'static) -> Self { | ||
#[cfg(any(feature = "http-proto", feature = "http-json"))] | ||
pub(crate) fn from_http(client: crate::exporter::http::OtlpHttpClient) -> Self { | ||
LogExporter { | ||
client: Box::new(client), | ||
client: LogExporterInner::Http(client), | ||
} | ||
} | ||
|
||
#[cfg(feature = "grpc-tonic")] | ||
pub(crate) fn from_tonic(client: crate::exporter::tonic::logs::TonicLogsClient) -> Self { | ||
LogExporter { | ||
client: LogExporterInner::Tonic(client), | ||
} | ||
} | ||
} | ||
|
||
#[async_trait] | ||
impl opentelemetry_sdk::export::logs::LogExporter for LogExporter { | ||
async fn export(&self, batch: LogBatch<'_>) -> LogResult<()> { | ||
self.client.export(batch).await | ||
#[allow(clippy::manual_async_fn)] | ||
fn export<'a>( | ||
&'a self, | ||
batch: &'a LogBatch<'a>, | ||
) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a { | ||
async move { | ||
match &self.client { | ||
#[cfg(feature = "grpc-tonic")] | ||
LogExporterInner::Tonic(client) => client.export(batch).await, | ||
#[cfg(any(feature = "http-proto", feature = "http-json"))] | ||
LogExporterInner::Http(client) => client.export(batch).await, | ||
} | ||
} | ||
} | ||
|
||
fn set_resource(&mut self, resource: &opentelemetry_sdk::Resource) { | ||
self.client.set_resource(resource); | ||
match &mut self.client { | ||
#[cfg(feature = "grpc-tonic")] | ||
LogExporterInner::Tonic(client) => client.set_resource(resource), | ||
#[cfg(any(feature = "http-proto", feature = "http-json"))] | ||
LogExporterInner::Http(client) => client.set_resource(resource), | ||
} | ||
} | ||
} |
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 annotation is required for false positive clippy warning - rust-lang/rust-clippy#12664
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.
Maybe have it on the project level until #12664 is addressed?
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.
Adding
#[allow(clippy::manual_async_fn)]
at the package level inCargo.toml
is not supported—it only works as an attribute in Rust source files. To suppress this lint effectively, it needs to be added inlib.rs
ormod.rs
for the main crate and separately forbenches
(and likelytests
as well). It seems, adding at method level seems cleaner as of now. We can revisit this if similar implementation is done for Spans, and the issue is not fixed by then.