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

add aws xray id generator #33

Merged
merged 11 commits into from
Feb 20, 2024
Merged

add aws xray id generator #33

merged 11 commits into from
Feb 20, 2024

Conversation

sreeo
Copy link
Contributor

@sreeo sreeo commented Feb 9, 2024

Fixes #
Design discussion issue (if applicable) #

Changes

open-telemetry/opentelemetry-rust#1489

Add Xray IdGenerator from opentelemetry-rust 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)

@sreeo sreeo requested a review from a team February 9, 2024 19:16
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (c40c650) 43.3% compared to head (03cc248) 45.0%.

Files Patch % Lines
opentelemetry-aws/src/trace/xray_propagator.rs 92.5% 15 Missing ⚠️
opentelemetry-aws/src/trace/id_generator.rs 93.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main     #33     +/-   ##
=======================================
+ Coverage   43.3%   45.0%   +1.7%     
=======================================
  Files         11      12      +1     
  Lines       1193    1236     +43     
=======================================
+ Hits         517     557     +40     
- Misses       676     679      +3     

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

@cijothomas
Copy link
Member

Can you add a changelog entry for this? Please state explicitly that this is ported over from the main package..

@sreeo
Copy link
Contributor Author

sreeo commented Feb 10, 2024

Can you add a changelog entry for this? Please state explicitly that this is ported over from the main package..

done

@lalitb
Copy link
Member

lalitb commented Feb 10, 2024

Just wondering if this should be part of opentelemetry-aws crate, or contrib crate - question to @open-telemetry/rust-maintainers ? Logically both AWS XRay Propagator and XRay IdGenerator can be within the same crate.

@cijothomas
Copy link
Member

Just wondering if this should be part of opentelemetry-aws crate, or contrib crate - question to @open-telemetry/rust-maintainers ? Logically both AWS XRay Propagator and XRay IdGenerator can be within the same crate.

opentelemetry-aws seems the logical option, but that crate is also unofficial. https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/main/opentelemetry-aws/src/lib.rs#L1
We need to see if anyone would like to own and maintain it, otherwise remove it.

@sreeo
Copy link
Contributor Author

sreeo commented Feb 13, 2024

We need to see if anyone would like to own and maintain it, otherwise remove it.

is this restricted to the maintainers or do you accept volunteers like me as well? @cijothomas

@cijothomas
Copy link
Member

We need to see if anyone would like to own and maintain it, otherwise remove it.

is this restricted to the maintainers or do you accept volunteers like me as well? @cijothomas

It is open to all, but there could be some requirements (which we haven't written down here yet). For example, one requirement is that you join the OpenTelemetry organization, and that you can respond to issues reported to the component in a reasonable time etc. It may take a while before we formally write these down, but until then we can continue with this crate.

@sreeo
Copy link
Contributor Author

sreeo commented Feb 14, 2024

okay

@cijothomas
Copy link
Member

Just wondering if this should be part of opentelemetry-aws crate, or contrib crate - question to @open-telemetry/rust-maintainers ? Logically both AWS XRay Propagator and XRay IdGenerator can be within the same crate.

@sreeo I think this is better option, than putting it into contrib crate and doing another move later (Its unclear what the future of contrib crate is. It was created when the contrib repo was not available, so may not be required). Could you move this into the aws crate itself in this PR?

@sreeo
Copy link
Contributor Author

sreeo commented Feb 16, 2024

sure, i'll move it to the aws crate.

update lint script

move xray id generator from contrib to aws crate
@hdost
Copy link
Contributor

hdost commented Feb 19, 2024

We need to see if anyone would like to own and maintain it, otherwise remove it.

is this restricted to the maintainers or do you accept volunteers like me as well? @cijothomas

It is open to all, but there could be some requirements (which we haven't written down here yet). For example, one requirement is that you join the OpenTelemetry organization, and that you can respond to issues reported to the component in a reasonable time etc. It may take a while before we formally write these down, but until then we can continue with this crate.

The general guidelines are here https://github.com/open-telemetry/community/blob/main/CONTRIBUTING.md this applies to the organization.

Our SIGs contribution guidelines are https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/main/CONTRIBUTING.md

However agreed we don't have a subset for these contrib crates. I think if we can have at least a set of approvers for the various contrib crates that would be great

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.

Thank you for the contribution!

Small change needed.

@@ -12,6 +12,7 @@

- Bump MSRV to 1.65 [#1318](https://github.com/open-telemetry/opentelemetry-rust/pull/1318)
- Bump MSRV to 1.64 [#1203](https://github.com/open-telemetry/opentelemetry-rust/pull/1203)
- Move Xray IdGenerator from `opentelemetry-rust` to `opentelemetry-contrib` [#33](https://github.com/open-telemetry/opentelemetry-rust-contrib/pull/33)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a leftover, should be in the opentelemetry-aws changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@lalitb
Copy link
Member

lalitb commented Feb 20, 2024

Can you also update the Readme.md for this opentelemetry-aws crate to indicate it contains both XRay propagator and XRay Id Generator. As of now, it says:

---clip

Supported component

Currently, this crate only supports XRay propagator. Contributions are welcome.

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.

Thanks for the fix.

@TommyCpp TommyCpp merged commit c4673aa into open-telemetry:main Feb 20, 2024
9 checks passed
@cijothomas
Copy link
Member

I think if we can have at least a set of approvers for the various contrib crates that would be great

This is the tricky part.. It is not sufficient for someone to be listed in codeowners file! They need to get write permission to the repo itself :( This is generally an issue in other otel repos too, with everyone doing own ways to manage it. We'll need to invent something of our own or follow some other languages. But this is not something we need to fix right away.

@cijothomas
Copy link
Member

Thanks @sreeo for taking care of this!

@sreeo sreeo deleted the id-generator branch February 22, 2024 10:16
@@ -29,6 +29,7 @@

- reduce `tokio` feature requirements #750
- Update to opentelemetry v0.18.0
- Move Xray IdGenerator from `opentelemetry-rust` to `opentelemetry-aws` [#33](https://github.com/open-telemetry/opentelemetry-rust-contrib/pull/33)
Copy link
Member

Choose a reason for hiding this comment

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

the entry got into the wrong place. Could you fix that in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #38

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.

5 participants