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

fix: AtomicI64, AtomicU64 for build mipsel processor #2301

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TGLuong
Copy link

@TGLuong TGLuong commented Nov 16, 2024

Fixes #
Design discussion issue (if applicable) #

Changes

  • Use portable-atomic for mipsel processor. Because when build for mipsel architecture, it don't have AtomicI64, AtomicU64 in std.

Please provide a brief description of the changes here.

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)

@TGLuong TGLuong requested a review from a team as a code owner November 16, 2024 09:20
Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.1%. Comparing base (69e7e89) to head (f000612).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2301   +/-   ##
=====================================
  Coverage   79.1%   79.1%           
=====================================
  Files        119     119           
  Lines      22555   22555           
=====================================
  Hits       17842   17842           
  Misses      4713    4713           

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

@TGLuong
Copy link
Author

TGLuong commented Nov 16, 2024

ref: #2054

@TGLuong TGLuong changed the title fix: AtomicI64, AtomicU64 for build mipsel processor [WIP] fix: AtomicI64, AtomicU64 for build mipsel processor Nov 16, 2024
@TGLuong TGLuong changed the title [WIP] fix: AtomicI64, AtomicU64 for build mipsel processor fix: AtomicI64, AtomicU64 for build mipsel processor Nov 16, 2024
@@ -18,6 +18,12 @@ pub(crate) use exponential_histogram::{EXPO_MAX_SCALE, EXPO_MIN_SCALE};
use once_cell::sync::Lazy;
use opentelemetry::{otel_warn, KeyValue};

#[cfg(any(target_arch = "mips", target_arch = "powerpc"))]
use portable_atomic::{AtomicBool, AtomicI64, AtomicU64, AtomicUsize};
Copy link
Member

Choose a reason for hiding this comment

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

are these the only place this SDK uses Atomics?

@brunobrolesi
Copy link

could you add this for xtensa arch too? I'm trying to use this sdk on ESP32 and getting this error:

error[E0432]: unresolved imports `std::sync::atomic::AtomicI64`, `std::sync::atomic::AtomicU64`

@cijothomas
Copy link
Member

@TGLuong Hi do you have bandwidth to continue working on this?

@cijothomas
Copy link
Member

@brunobrolesi Would you be willing to take over and continue this work? We can give @TGLuong few days to respond, and then you can pick this up, if you are okay!

@TGLuong TGLuong force-pushed the fix-build-mipsel-with-atomic-fail branch from c556cca to f000612 Compare February 6, 2025 04:51
@brunobrolesi
Copy link

brunobrolesi commented Feb 9, 2025

@cijothomas Sorry for the delay, I did get around to implementing the fix locally, but I ran into more problems that will prevent me from continuing development for the time being. For the xtensa architecture, these problems exist in the hyper and socket 2 libs and prevent the use of otel with exporters that use http.

hyperium/hyper#3684
rust-lang/socket2#516

Note: Adding target_arch = “xtensa” to the same places where @TGLuong had added the validations allowed me to use the stdout exporter

@cijothomas
Copy link
Member

@cijothomas Sorry for the delay, I did get around to implementing the fix locally, but I ran into more problems that will prevent me from continuing development for the time being. For the xtensa architecture, these problems exist in the hyper and socket 2 libs and prevent the use of otel with exporters that use http.

hyperium/hyper#3684 rust-lang/socket2#516

Note: Adding target_arch = “xtensa” to the same places where @TGLuong had added the validations allowed me to use the stdout exporter

Thanks for checking. Looks like these architectures could use a formal, production ready stdout exporter - #2602

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