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

misc. backports #1361

Merged
merged 7 commits into from
Apr 17, 2021
Merged

misc. backports #1361

merged 7 commits into from
Apr 17, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 16, 2021

This cherry-picks a number of recent commits from master to v0.1.x:

jtescher and others added 5 commits April 15, 2021 16:37
## Motivation

Understand the overhead added by recording OpenTelemetry data so that it
can be minimized.

## Solution

Add criterion benchmarks, initially tracking request/response style
workloads (1 main span with 99 children).

This patch adds benchmarks for standard `tracing-opentelemetry` usage,
as well as baselines for understanding the overhead specific to the
usage of the otel tracer, and registry span access patterns.
* subscriber: fix span data for new, exit, and close events

New, exit and close span events are generated while the current
context is set to either `None` or the parent span of the span the
event relates to. This causes spans data to be absent from the JSON
output in the case of the `None`, or causes the span data to reference
the parent's span data. Changing the way the current span is
determined allows the correct span to be identified for these
events. Trying to access the events `.parent()` allows access of the
correct span for the `on_event` actions, while using `.current_span()`
works for normal events.

Ref: #1032

* Fix style

* subscriber: improve test for #1333

Based on feedback by @hawkw, I've improved the test for #1333 to parse
the json output. This is more specifc for the bug and allows easier
testing of the different span `on_events`.

Ref: #1333 (review)

* subscriber: improve #1334 tests covering all span states

Use the `on_records` test method check all events have the correct
context as described in the PR.

* Apply suggestions from code review

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
The following two patterns for the `event!()` are identical to each
other, and the second one swapped the order of `$lvl` and `parent:
$parent` which I don't think is correct. 
```rust
    (parent: $parent:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: module_path!(),
            parent: $parent,
            $lvl,
            { message = format_args!($($arg)+), $($fields)* }
        )
    );
    (parent: $parent:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: module_path!(),
            $lvl,
            parent: $parent,
            { message = format_args!($($arg)+), $($fields)* }
        )
    );
```

I found the duplicated via
[macro_railraod](https://github.com/lukaslueg/macro_railroad). Here is
the diagram: 

![image](https://user-images.githubusercontent.com/3369694/114296705-47ec8700-9adf-11eb-971f-f485329d0339.png)
## Motivation

Allow users to set custom span status codes and messages to follow
opentelemetry semantic conventions.

## Solution

Support the status code and status message fields in
`OpenTelemetrySubscriber`

Proposal to close #1346
@hawkw hawkw requested review from jtescher and a team as code owners April 16, 2021 21:08
@hawkw hawkw force-pushed the eliza/backport-a-bunch-fstuff branch 2 times, most recently from d45b6c5 to e2f7324 Compare April 16, 2021 21:44
hawkw and others added 2 commits April 16, 2021 16:57
## Motivation

Currently, the default `Compact` and `Full` formatters in
`tracing-subscriber` will prefix log lines with a single space when
timestamps are disabled. The space is emitted in order to pad the
timestamp from the rest of the log line, but it shouldn't be emitted
when timestamps are turned off. This should be fixed.

## Solution

This branch fixes the issue by skipping `time::write` entirely when
timestamps are disabled. This is done by tracking an additional boolean
flag for disabling timestamps.

Incidentally, this now means that span lifecycle timing can be enabled
even if event timestamps are disabled, like this:
```rust
use tracing_subscriber::fmt;
let subscriber = fmt::subscriber()
    .without_time()
    .with_timer(SystemTime::now)
    .with_span_events(fmt::FmtSpan::FULL);
```
or similar.

I also added a new test reproducing the issue, and did a little
refactoring to try and clean up the timestamp formatting code a bit.

Closes #1354
## Motivation

Exposing an accessor for the `FieldSet` on `Attributes` can motivate the
subscriber to achieve some performance improvement, such as
`OpenTelemetrySubscriber` #1327.

## Alternative

Make `ValueSet::field_set()` be `pub` instead of `pub(crate)`.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/backport-a-bunch-fstuff branch from e2f7324 to 34b0bd8 Compare April 16, 2021 23:59
@hawkw hawkw merged commit 95fe9a3 into v0.1.x Apr 17, 2021
@hawkw hawkw deleted the eliza/backport-a-bunch-fstuff branch April 17, 2021 21:48
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.

6 participants