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

Use a builder pattern for loggers and tracers #1567

Merged
merged 18 commits into from
Apr 21, 2024

Conversation

izquierdo
Copy link
Contributor

Fixes #1527
Design discussion issue (if applicable) #1527

Changes

This is currently just an exploratory PR to aid in the design discussion of the attached issue. May upgrade this PR and submit for review depending on the discussion result.

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)

@cijothomas
Copy link
Member

@izquierdo Do you want us to review now, or should we wait till PR is marked non-draft?

@izquierdo
Copy link
Contributor Author

@izquierdo Do you want us to review now, or should we wait till PR is marked non-draft?

Please wait until the PR is marked as non-draft.

However, I made a comment here: #1527 (comment)

Copy link

codecov bot commented Mar 3, 2024

Codecov Report

Attention: Patch coverage is 76.56904% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 69.0%. Comparing base (f203b03) to head (6498a95).
Report is 9 commits behind head on main.

❗ Current head 6498a95 differs from pull request most recent head 68ce4bc. Consider uploading reports for the commit 68ce4bc to get more accurate results

Files Patch % Lines
opentelemetry/src/logs/logger.rs 64.7% 12 Missing ⚠️
opentelemetry/src/trace/tracer_provider.rs 64.7% 12 Missing ⚠️
opentelemetry-zipkin/src/exporter/mod.rs 0.0% 10 Missing ⚠️
opentelemetry-otlp/src/logs.rs 0.0% 8 Missing ⚠️
opentelemetry-jaeger/src/exporter/config/mod.rs 0.0% 5 Missing ⚠️
opentelemetry-otlp/src/span.rs 50.0% 5 Missing ⚠️
opentelemetry-appender-log/src/lib.rs 0.0% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1567     +/-   ##
=======================================
- Coverage   69.3%   69.0%   -0.3%     
=======================================
  Files        136     136             
  Lines      19637   19587     -50     
=======================================
- Hits       13610   13525     -85     
- Misses      6027    6062     +35     

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

@izquierdo izquierdo force-pushed the issue-1527 branch 3 times, most recently from a3648ad to e41fc1e Compare March 11, 2024 21:18
@izquierdo izquierdo force-pushed the issue-1527 branch 2 times, most recently from 2442b8e to 66fe3e6 Compare March 18, 2024 21:28
@izquierdo izquierdo changed the title [WIP] Try using a builder pattern for all signal providers Use a builder pattern for loggers and tracers Mar 19, 2024
@izquierdo izquierdo marked this pull request as ready for review March 19, 2024 22:01
@izquierdo izquierdo requested a review from a team March 19, 2024 22:01
@izquierdo
Copy link
Contributor Author

Let me open this for review as is, with loggers and tracers. Will leave metrics out of this PR.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Seems aligned with the discussion in #1527 overall looks good

opentelemetry/src/common.rs Outdated Show resolved Hide resolved
opentelemetry/src/trace/tracer_provider.rs Outdated Show resolved Hide resolved
Co-authored-by: Zhongyang Wu <zhongyang.wu@outlook.com>
@TommyCpp TommyCpp self-assigned this Apr 2, 2024
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM. We probably want to revise the change log as suggested above

@TommyCpp TommyCpp removed their assignment Apr 15, 2024
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks!
I left couple of nit comments to improve changelog, but Looks good overall.
Will you help with replicating this for Meter as well?

cijothomas and others added 6 commits April 17, 2024 17:00
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
@izquierdo
Copy link
Contributor Author

Will you help with replicating this for Meter as well?

Yes! But in a separate PR

@cijothomas cijothomas merged commit 7a51e75 into open-telemetry:main Apr 21, 2024
15 checks passed
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.

Instrumentation scope - move to builder pattern?
5 participants