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.

Sign up for GitHub

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

Jump to bottom

fix: Backport opentelemetry 0.18 code from main repo #9

Merged
merged 3 commits into from
May 22, 2023
Merged

fix: Backport opentelemetry 0.18 code from main repo #9

merged 3 commits into from
May 22, 2023

Conversation

jaysonsantos
Copy link
Contributor

Motivation

I noticed that this repo got the cut-off before opentelemetry 0.18 was committed into the main repo so I tried to backport what was there.

Solution

This is pretty much a backport from the code that @jtescher did on the main repo.

Let me know if you see something wrong, I tried to adapt the rejections the best way possible.

This fix #8

@jaysonsantos jaysonsantos requested a review from jtescher as a code owner March 25, 2023 10:50
@jaysonsantos jaysonsantos changed the title fix: Backport code from main repo fix: Backport opentelemetry 0.18 code from main repo Mar 25, 2023
@jtescher @jaysonsantos
fix: Backport code from main repo
6b86ab6 
This is pretty much a backport from the code that @jtescher did on the main repo

Signed-off-by: Jayson Reis <[email protected]>
@jaysonsantos jaysonsantos force-pushed the backport-opentelemetry-0.18 branch from 2d3d3ec to 6b86ab6 Compare March 25, 2023 10:54
jaysonsantos
jaysonsantos commented Mar 25, 2023
View reviewed changes
Copy link
Contributor Author

@jaysonsantos jaysonsantos left a comment

Choose a reason for hiding this comment

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

I just left comments on parts of the code that seems that the current version should stay, would you mind taking a look at them?

src/layer.rs
@@ -1104,7 +1119,7 @@ mod tests {
let subscriber = tracing_subscriber::registry().with(layer().with_tracer(tracer.clone()));

tracing::subscriber::with_default(subscriber, || {
tracing::debug_span!("request", otel.kind = %SpanKind::Server);
tracing::debug_span!("request", otel.kind = "server");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
tracing::debug_span!("request", otel.kind = "server");
tracing::debug_span!("request", otel.kind = %SpanKind::Server);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The spec doesn't define a Display impl so this was removed in the opentelemetry crate as it pushes for a 1.0 release, so we have to provide our own conventions in this crate for now.

src/layer.rs
@@ -1154,7 +1178,7 @@ mod tests {
let _g = existing_cx.attach();

tracing::subscriber::with_default(subscriber, || {
tracing::debug_span!("request", otel.kind = %SpanKind::Server);
tracing::debug_span!("request", otel.kind = "server");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
tracing::debug_span!("request", otel.kind = "server");
tracing::debug_span!("request", otel.kind = %SpanKind::Server);

src/tracer.rs

// Sample or defer to existing sampling decisions
let (flags, trace_state) = if let Some(result) = &builder.sampling_result {
process_sampling_result(result, parent_trace_flags)
} else if no_parent || remote_parent {
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the previous branch stay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no this is right

src/tracer.rs
@@ -95,12 +95,6 @@ impl PreSampledTracer for Tracer {
builder.sampling_result.as_ref().unwrap(),
parent_trace_flags,
)
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this also stay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This changed as well

@djc
Copy link
Collaborator

djc commented Mar 27, 2023

Note that opentelemetry 0.19 has been released yesterday.

@jaysonsantos
Copy link
Contributor Author

@djc I've tested 0.19 with this PR, and it seems to work fine but, as this is a backport from another author, I didn't want to change the code in this PR as I don't “own” it

@davidbarsky
Copy link
Member

davidbarsky commented Mar 28, 2023 •
edited
Loading

Thanks for doing this PR! Sorry for the delay in reviewing this. I'll get on it shortly.

I've tested 0.19 with this PR, and it seems to work fine but, as this is a backport from another author, I didn't want to change the code in this PR as I don't “own” it

I think that for clarity of commit history, we should make upgrading to opentelemetry 0.19 a separate PR.

@jaysonsantos jaysonsantos mentioned this pull request Apr 1, 2023
Merged
jtescher
jtescher approved these changes Apr 11, 2023
View reviewed changes
src/tracer.rs

// Sample or defer to existing sampling decisions
let (flags, trace_state) = if let Some(result) = &builder.sampling_result {
process_sampling_result(result, parent_trace_flags)
} else if no_parent || remote_parent {
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no this is right

src/tracer.rs
@@ -95,12 +95,6 @@ impl PreSampledTracer for Tracer {
builder.sampling_result.as_ref().unwrap(),
parent_trace_flags,
)
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changed as well

@RenanSFreitas
Copy link

RenanSFreitas commented Apr 26, 2023 •
edited
Loading

Hey,

Any chances this gets merged soon? I'd like to use some features present in opentelemetry 0.19.0 together with tracing-opentelemetry. Thank you all o/

@xlc xlc mentioned this pull request Apr 27, 2023
Closed
@tamasd tamasd mentioned this pull request May 1, 2023
Closed
@112batuhan 112batuhan mentioned this pull request May 1, 2023
Open
@shaun-cox
Copy link
Contributor

@jaysonsantos, looks like examples/opentelemetry.rs needs to be updated as well to use opentelemetry_jaeger::new_agent_pipeline() instead of opentelemetry_jaeger::new_pipeline() in order to fix the above checks?

I'm eager to see this merged too (as well as the update for opentelemetry 0.19). If there is anything I can do to help, please let me know... Thanks for shepherding this!

@jaysonsantos jaysonsantos requested a review from jtescher May 3, 2023 17:17
@jaysonsantos
Copy link
Contributor Author

@davidbarsky @jtescher do you mind taking a look on the latest commit?

@ttys3 ttys3 mentioned this pull request May 4, 2023
Merged
@abernix abernix mentioned this pull request May 15, 2023
Open
@Dzejkop
Copy link

Dzejkop commented May 17, 2023

@davidbarsky @jtescher do you know when you'll be able to take a look at this? I'm wondering if I should wait or use a temporary fork

@jtescher jtescher merged commit a5c27f6 into tokio-rs:v0.1.x May 22, 2023
@jtescher
Copy link
Collaborator

Thanks @jaysonsantos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtescher jtescher Awaiting requested review from jtescher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update opentelemetry to 0.18
7 participants
@jaysonsantos @djc @davidbarsky @RenanSFreitas @shaun-cox @Dzejkop @jtescher