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

Span의 Start/End Time을 임의로 지정할 수 있는 기능 추가 #1

Open
wants to merge 1 commit into
base: tracing-opentelemetry
Choose a base branch
from

Conversation

libc-furiosa
Copy link
Collaborator

Motivation

  • 기존 구현에서는 Span의 StartTime과 EndTime이 span의 life cycle에 의해서만 지정되도록 제한되어 있음
    • on_new_span(): 호출 시점의 SystemTime::now()로 start_time 정해짐
    • on_close() : 호출 시점의 SystemTime::now()로 end_time 정해짐
  • 일반적인 함수 콜로 정의되는 span이 아닌, NPU Execution, Instruction의 경우 Span의 start ~ end를 계산에 의해 산출된 값으로 지정할 필요가 있어 SystemTime::now()가 아닌 임의의 값을 다룰 수 있어야 함

Solution

  • 사전에 정의한 특수한 키(npu.start, npu.end)에 정해진 형식(nanos, u128)을 입력하는 경우 start_time, end_time으로 지정한 값이 사용되도록 기능을 추가함

Example

// start, end: Duration (since UNIX_EPOCH)

let span = info_span!("NPU Instruction", name,
        "npu.start" = start.as_nanos(), "npu.end" = end.as_nanos(),
        cat, base, pe_idx, exec_idx, inst_idx);
drop(span);

///

let span = info_span!("NPU Task", name = "Execution",
        "npu.start" = start.as_nanos(), "npu.end" = field::Empty,
        cat, base, pe_idx, exec_idx);
// ...
span.record("npu.end", end.as_nanos());
drop(span);

@libc-furiosa libc-furiosa requested a review from a team August 11, 2022 06:05
@libc-furiosa libc-furiosa self-assigned this Aug 11, 2022
/// [`Span`]: opentelemetry::trace::Span
fn record_u128(&mut self, field: &field::Field, value: u128) {
match field.name() {
SPAN_NPU_START_TIME_FIELD => {
Copy link

Choose a reason for hiding this comment

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

저도 구체적인 아이디어가 있는건 아닌데 혹시 이 부분이 조금 더 general하게 넣을수 있는 API 포인트가 있을까요? 예를 들어 지금은 npu.start 라는 특정 string이 field에 들어가는 형태인데 다른 제 3의 방법이 있을까 해서요 (아마 없을거 같긴한데.. 😇 업스트림에 넣는게 계속 조금 아쉬워서 질문드립니다.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아예 시간 정보를 attribute로 받지 않고, span에 대한 function call로 builder의 time을 update하는 방법을 찾았습니다. SpanExt trait을 만들고 Span에 대해 구현을 아래와 같이 하면 되는 것 같습니다.

// impl OpenTelemetrySpanExt for tracing::Span
    fn set_start_time(&self, start: SystemTime) {
        self.with_subscriber(|(id, subscriber)| {
            if let Some(get_context) = subscriber.downcast_ref::<WithContext>() {
                get_context.with_context(subscriber, id, |otel_data, _tracer| {
                    otel_data.builder.start_time = Some(start);
                })
            }
        });
    }
// ...
let span = info_span!("something", ...);
span.set_start_time(start_time);

다만 유일하게 문제가 되는 부분이 아래의 on_close() 부분인데요.
span을 drop하면 사실상 항상 호출되는 on_close에서 end_time을 now로 덮어버려서 set_end_time이 의미가 없게 되어버립니다.
이 부분만 회피할 수 있다면 tracing-opentelemetry를 수정하지 않아도 될 것 같습니다. 다만 해결할 수 있는 뾰족한 방법이 보이지 않네요.

.start_with_context(&self.tracer, &parent_cx);
if builder.end_time.is_none() {
// Assign end time
builder = builder.with_end_time(SystemTime::now());
Copy link

Choose a reason for hiding this comment

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

realtime clock을 쓰는 부분은 monotonic clock이랑 conflict가 없는지 확인해봐야 될 거 같아요. 저희는 지금 monotonic clock을 쓰고 있거든요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇지 않아도 이 부분을 npu-tools에서 리뷰를 요청드리려 했었습니다.
Opentelemetry Span에서는 SystemTime을 기준으로 하다보니 Instant를 그대로 사용할 수는 없었는데요.
instrument의 경우 일반적인 방식으로 on_new_span과 on_close 시점의 SystemTime::now()가 적용되고,
NPU profiling의 경우 profiler.sync() 시 SystemTime::now() (기존의 Instant::now()를 대체)가 불리고 이를 기준으로 상대 시간으로 계산하고 있습니다.

기존 방식과 결과 자체는 다르지 않아 보이는데 검토는 필요할 것 같습니다. 이 내용은 npu-tools 쪽에 올릴 PR에서 자세히 다루면 좋을 것 같습니다.

SunghwanShim pushed a commit to SunghwanShim/tracing that referenced this pull request Aug 1, 2023
## Motivation

In my library I define a `macro_rules! concat` macro, i.e.
[`callbag::concat`](https://docs.rs/callbag/latest/callbag/macro.concat.html).

When I try to call `tracing::info!(...)`, I get error output such as
this:

<details>
<summary>error output</summary>

<!-- leave a blank line above -->
```
> RUSTFLAGS='-Z macro-backtrace' cargo +nightly clippy --features trace
    Checking callbag v0.14.0 (/home/teohhanhui/projects/teohhanhui/callbag-rs)
error[E0308]: mismatched types
  --> src/concat.rs:89:9
   |
89 |         info!("from sink: {message:?}");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `u32`

error[E0277]: the trait bound `std::sync::Arc<core::Callbag<never::Never, _>>: std::convert::From<&str>` is not satisfied
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `std::sync::Arc<core::Callbag<never::Never, _>>`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (tokio-rs#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (furiosa-ai#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (furiosa-ai#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (furiosa-ai#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (tokio-rs#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (tokio-rs#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (furiosa-ai#3)
     |   in this expansion of `$crate::event!` (tokio-rs#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (furiosa-ai#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (furiosa-ai#2)
     |
     = help: the following implementations were found:
               <std::sync::Arc<B> as std::convert::From<std::borrow::Cow<'a, B>>>
               <std::sync::Arc<T> as std::convert::From<T>>
               <std::sync::Arc<T> as std::convert::From<std::boxed::Box<T>>>
               <std::sync::Arc<[T]> as std::convert::From<&[T]>>
             and 9 others
     = note: required because of the requirements on the impl of `std::convert::Into<std::sync::Arc<core::Callbag<never::Never, _>>>` for `&str`
note: required by a bound in `concat::concat`
    --> src/concat.rs:81:8
     |
72   | pub fn concat<
     |        ------ required by a bound in this
...
81   |     S: Into<Arc<Source<T>>> + Send + Sync,
     |        ^^^^^^^^^^^^^^^^^^^^ required by this bound in `concat::concat`

error[E0308]: mismatched types
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found struct `core::Callbag`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (tokio-rs#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (furiosa-ai#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (furiosa-ai#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (furiosa-ai#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (tokio-rs#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (tokio-rs#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (furiosa-ai#3)
     |   in this expansion of `$crate::event!` (tokio-rs#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (furiosa-ai#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (furiosa-ai#2)
     |
     = note: expected reference `&'static str`
                   found struct `core::Callbag<never::Never, _>`

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `callbag` due to 3 previous errors
```
</details>

This is because of my `concat` macro being in scope.

## Solution

This branch adds a re-export of `core::concat!` in the
`__macro_support` module, and changes all the `tracing` macros to use
that, rather than using an un-namespaced `concat!`. The re-export
ensures that everything still works even in a crate that redefines the
`core` name to something else.

Co-authored-by: Eliza Weisman <[email protected]>
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.

2 participants