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

Move opentelemetry_api code back into opentelemetry and remove the former #1226

Merged
merged 6 commits into from
Aug 28, 2023

Conversation

shaun-cox
Copy link
Contributor

@shaun-cox shaun-cox commented Aug 24, 2023

Fixes #1186

Changes

As part of #1199, @djc asked:

At this point, should we just move all of the opentelemetry_api code back into the opentelemetry directory?

This PR delivers that. To make reviewing hopefully easier, I've split it into several commits that can be squashed when merged and reviewed independently:

  • commit 1
    • change Cargo.toml dependencies from opentelemetry_api to opentelemetry
    • replace use opentelemetry_api:: with use opentelemetry::
  • commit 2
    • moves opentelemetry-api src directory to opentelemetry and removes the former.
    • opentelemetry/src/lib.rs manually combined
  • commit 3
    • updates CHANGELOG.md files to remove mentions of opentelemetry-api and just use opentelemetry
    • (maybe this isn't appropriate, in which case I'll revert.)
  • commit 4
    • manually combined opentelemetry/CHANGELOG.md with the old opentelemetry-api/CHANGELOG.md and adjusted opentelemtry/README.md to mention the related opentelemetry_sdk crate.
  • commit 5
    • removed opentelemetry-api directory

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 50.0% and no project coverage change.

Comparison is base (1688ec6) 49.3% compared to head (87d75d8) 49.3%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1226   +/-   ##
=====================================
  Coverage   49.3%   49.3%           
=====================================
  Files        174     173    -1     
  Lines      21038   21008   -30     
=====================================
- Hits       10380   10371    -9     
+ Misses     10658   10637   -21     
Files Changed Coverage Δ
opentelemetry-otlp/src/exporter/grpcio/logs.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/grpcio/metrics.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/grpcio/mod.rs 30.4% <0.0%> (ø)
opentelemetry-otlp/src/exporter/grpcio/trace.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/http/logs.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/http/metrics.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/http/mod.rs 0.0% <0.0%> (ø)
opentelemetry-otlp/src/exporter/http/trace.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/tonic/logs.rs 0.0% <ø> (ø)
opentelemetry-otlp/src/exporter/tonic/metrics.rs 0.0% <ø> (ø)
... and 88 more

... and 1 file with indirect coverage changes

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

@shaun-cox shaun-cox force-pushed the api_crate branch 2 times, most recently from 0a85091 to e494400 Compare August 24, 2023 16:15
@shaun-cox shaun-cox marked this pull request as ready for review August 24, 2023 18:25
@shaun-cox shaun-cox requested a review from a team August 24, 2023 18:25
@shaun-cox shaun-cox changed the title use opentelemetry instead of opentelemetry_api Move opentelemetry_api code back into opentelemetry and remove the former Aug 24, 2023
@@ -1,28 +0,0 @@
![OpenTelemetry — An observability framework for cloud-native software.][splash]
Copy link
Member

Choose a reason for hiding this comment

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

it may be a good idea to to keep this file, with a note saying "its no longer maintained and users should use opentelemetry crate instead". Or maybe that can be done in crates.io description manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 we will need to figure out a way to prompt users to opentelemetry crate when they end up on the opentelemetry_api crate

Copy link
Contributor

Choose a reason for hiding this comment

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

Could submit an informational RustSec security advisory setting the status for opentelemetry_api to unmaintained.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've submitted rustsec/advisory-db#1994 and a PR

In addition to `opentelemetry`, which only carries the API, the
[`open-telemetry/opentelemetry-rust`] repository contains several additional
crates designed to be used with the `opentelemetry` ecosystem. This includes a
collection of trace `SpanExporter` and metrics pull and push controller
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is outdated. will send a follow up to address it after this PR.

@cijothomas
Copy link
Member

commit 3
updates CHANGELOG.md files to remove mentions of opentelemetry-api and just use opentelemetry
(maybe this isn't appropriate, in which case I'll revert.)

mm.. I don't think we need to update past changelogs.

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.

LGTM. I left a couple of nits, and also do not think we need commit3 to update old changelog entries.

Thanks a lot for this, and really appreciate the way step-by-step commits is done to make reviewing much easier!

@lalitb
Copy link
Member

lalitb commented Aug 24, 2023

commit 3
updates CHANGELOG.md files to remove mentions of opentelemetry-api and just use opentelemetry
(maybe this isn't appropriate, in which case I'll revert.)

mm.. I don't think we need to update past changelogs.

Same thought. Do we need to update old changelogs entries ?

`opentelemetry` ecosystem. This includes a collection of trace `SpanExporter`
and metrics pull and push controller implementations, as well as utility and
adapter crates to assist in propagating state and instrumenting applications.
In addition to `opentelemetry`, which only carries the API, the
Copy link
Member

@lalitb lalitb Aug 24, 2023

Choose a reason for hiding this comment

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

Should opentelemetry/README.md now only contain the information specific to API. Most of the content is similar to the root level README.md, and can be removed. Probably something to be addressed once this PR is merged.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Nicely done. Thanks.

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Glad people are finally on board with this.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working through this -- must have been pretty tedious.

opentelemetry/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Awesome thanks @shaun-cox

@jtescher jtescher merged commit 42e58fc into open-telemetry:main Aug 28, 2023
12 checks passed
@shaun-cox shaun-cox deleted the api_crate branch August 28, 2023 18:16
KaczDev pushed a commit to KaczDev/opentelemetry-rust that referenced this pull request Aug 28, 2023
TommyCpp added a commit that referenced this pull request Sep 2, 2023
* feat: addInMemoryLogExporter

* linting

* Update CHANGELOG.md

* changed finished to emitted logs, moved example

* replace clone_log with cloned()

* remove proj example in favor of single file

* corrected dependency, removed repeated code

* changed finished to emitted logs, moved example

* remove proj example in favor of single file

* corrected dependency, removed repeated code

* corrected dependency, removed repeated code

* fix: endpoint urls for otlp http exporter. (#1210)

* Move opentelemetry_api code back into opentelemetry and remove the former (#1226)

* [user_events log exporter] Upgrade eventheader-dynamic dependency (#1230)

* feat: addInMemoryLogExporter

* linting

* Update CHANGELOG.md

* remove the example from examples

* fixes after rebase

* missing doc, added no_run in examples

* fix ci test(stable) and docs

* more examples ci fixes

* added dev-dependencies to resolve ci issue

* corrected required features

* remove comments about returning error

* Update example description

Co-authored-by: Cijo Thomas <[email protected]>

---------

Co-authored-by: Zhongyang Wu <[email protected]>
Co-authored-by: Shaun Cox <[email protected]>
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
philipcristiano added a commit to philipcristiano/nostress that referenced this pull request Nov 21, 2023
philipcristiano added a commit to philipcristiano/hello_idc that referenced this pull request Nov 21, 2023
philipcristiano added a commit to philipcristiano/docker-prefetch-image that referenced this pull request Nov 21, 2023
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.

What is the difference between the API and SDK?
8 participants