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 OrderMap module to root of otel-api crate #1061

Merged
merged 6 commits into from
May 16, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented May 9, 2023

Fixes #
Design discussion issue (if applicable) #

Changes

OrderMap module is currently stored under crate::opentelemetry-api::trace hierarchy. Going ahead as part of #788, this data structure would also be used in logs bridge-api/sdk to log attributes. As this is a generic data structure, and doesn't logically belong to Trace, moving it to root of opentelemetry-api crate. This will enable it to used in both trace and logs in much cleaner way.

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)

@lalitb lalitb requested a review from a team May 9, 2023 23:29
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ecf0dda) 54.9% compared to head (821ec85) 54.9%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1061   +/-   ##
=====================================
  Coverage   54.9%   54.9%           
=====================================
  Files        151     151           
  Lines      18242   18242           
=====================================
  Hits       10025   10025           
  Misses      8217    8217           
Impacted Files Coverage Δ
opentelemetry-api/src/lib.rs 100.0% <ø> (ø)
opentelemetry-api/src/order_map.rs 4.2% <ø> (ø)
opentelemetry-api/src/trace/mod.rs 80.8% <ø> (ø)
opentelemetry-api/src/trace/tracer.rs 45.6% <ø> (ø)
opentelemetry-sdk/src/trace/sampler.rs 92.4% <ø> (ø)
...try-sdk/src/trace/sampler/jaeger_remote/sampler.rs 6.4% <ø> (ø)
opentelemetry-sdk/src/trace/tracer.rs 93.4% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Looks good, thanks. Could you add a CHANGELOG?

this may be breaking change for existing trace users as they have to change the specified path of this module in their code

One way to mitigate it is to re-export it in traces module

@lalitb
Copy link
Member Author

lalitb commented May 10, 2023

Looks good, thanks. Could you add a CHANGELOG?
One way to mitigate it is to re-export it in traces module

Thanks, added changelog, and re-export in trace module.

@TommyCpp TommyCpp merged commit 69fd972 into open-telemetry:main May 16, 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.

3 participants