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.

Sign up for GitHub

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

Jump to bottom

feat: replace OtlpPipeline with exporter builders #2221

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: replace OtlpPipeline with exporter builders #2221

wants to merge 1 commit into from
+744 −873

Conversation

pitoniak32
Copy link
Contributor

Fixes #1810

Changes

Started to replace the OTLPPipeline pattern in the opentelemetry-otlp crate.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@pitoniak32
Copy link
Contributor Author

pitoniak32 commented Oct 19, 2024 •
edited
Loading

I have started looking into replicating opentelemetry-stdout patterns. I have gotten to a place that with very basic and defaulted behavior its similar. I just wanted to confirm that it seems reasonable before I start getting into the details of allowing more customization to the exporter builders.

If you have a chance to review this @cijothomas I would really appreciate it!

@pitoniak32 pitoniak32 marked this pull request as ready for review October 19, 2024 12:57
@pitoniak32 pitoniak32 requested a review from a team as a code owner October 19, 2024 12:57
@pitoniak32
Copy link
Contributor Author

This might be another issue / PR, but I'm noticing the SdkMeterProvider and TracerProvider have different interfaces for configuring options like Resource for example. I have a few ideas for possibly unifying this if its open for discussion in another location?

@pitoniak32 pitoniak32 force-pushed the feat/normalize-otlp-crate branch 3 times, most recently from 414dfac to dfeb006 Compare October 19, 2024 18:50
@codecov Codecov
Copy link

codecov bot commented Oct 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 55.73123% with 112 lines in your changes missing coverage. Please review.

Project coverage is 79.7%. Comparing base (4852a5e) to head (b4b5118).

Files with missing lines Patch % Lines
opentelemetry-otlp/src/logs.rs 0.0% 38 Missing ⚠️
opentelemetry-otlp/src/metric.rs 0.0% 29 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/mod.rs 67.5% 24 Missing ⚠️
opentelemetry-otlp/src/span.rs 53.8% 12 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs 89.0% 6 Missing ⚠️
opentelemetry-otlp/src/exporter/mod.rs 62.5% 3 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##            main   #2221     +/-   ##
=======================================
+ Coverage   79.3%   79.7%   +0.4%     
=======================================
  Files        121     121             
  Lines      20944   20858     -86     
=======================================
+ Hits       16612   16640     +28     
+ Misses      4332    4218    -114

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

@cijothomas
Copy link
Member

This might be another issue / PR, but I'm noticing the SdkMeterProvider and TracerProvider have different interfaces for configuring options like Resource for example. I have a few ideas for possibly unifying this if its open for discussion in another location?

Yes, ensuring consistency across signals wherever feasible is definitely something we need to tackle soon!

cijothomas
cijothomas reviewed Oct 20, 2024
View reviewed changes
examples/self-diagnostics/src/main.rs
.unwrap();

let provider = opentelemetry_sdk::logs::LoggerProvider::builder()
.with_batch_exporter(exporter, opentelemetry_sdk::runtime::Tokio)
Copy link
Member

Choose a reason for hiding this comment

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

I like this direction.

cijothomas
cijothomas reviewed Oct 20, 2024
View reviewed changes
opentelemetry-sdk/src/metrics/exporter.rs
@@ -11,7 +11,7 @@ use super::data::Temporality;
///
/// This is the final component in the metric push pipeline.
#[async_trait]
pub trait PushMetricsExporter: Send + Sync + 'static {
pub trait PushMetricExporter: Send + Sync + 'static {
Copy link
Member

Choose a reason for hiding this comment

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

lets do this in its own PR, to keep the scope for each PR very focused.

cijothomas
cijothomas reviewed Oct 20, 2024
View reviewed changes
opentelemetry-stdout/examples/basic.rs
@@ -28,7 +28,7 @@ static RESOURCE: Lazy<Resource> = Lazy::new(|| {
fn init_trace() {
let exporter = opentelemetry_stdout::SpanExporter::default();
let provider = TracerProvider::builder()
.with_simple_exporter(exporter)
.with_batch_exporter(exporter, runtime::Tokio)
Copy link
Member

Choose a reason for hiding this comment

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

lets keep it simple itself. stdout is only for learning purposes. and simple exporter is also only meant for that.

cijothomas
cijothomas reviewed Oct 20, 2024
View reviewed changes
opentelemetry-otlp/src/exporter/mod.rs
@@ -66,7 +66,7 @@ pub(crate) mod tonic;

/// Configuration for the OTLP exporter.
#[derive(Debug)]
pub struct ExportConfig {
pub struct ExporterConfig {
Copy link
Member

Choose a reason for hiding this comment

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

also suggest to keep the renaming to a separate, small pr, so it is easier to review.

@cijothomas
Copy link
Member

I have started looking into replicating opentelemetry-stdout patterns. I have gotten to a place that with very basic and defaulted behavior its similar. I just wanted to confirm that it seems reasonable before I start getting into the details of allowing more customization to the exporter builders.

If you have a chance to review this @cijothomas I would really appreciate it!

Took a brief look and I like this direction. Left some comments, mostly about limiting the scope of PR. We really prefer short PRs each solving one particular aspect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OTLPExporter Pipeline issues
2 participants
@pitoniak32 @cijothomas