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

Initial opentracing bridge #98

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Initial opentracing bridge #98

merged 3 commits into from
Sep 25, 2019

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Aug 15, 2019

It's very basic. There is a bunch of TODOs. They are more or less:

  • How to manage context.Context? In opentracing, you basically create a span and then call opentracing.ContextWithSpan to get the context with the span stored in it. In opentelemetry, you first create a context, then you create a span (creating a span requires passing a context).
  • Support baggage items, injection, extraction
  • Links setup - when creating a span in opentracing, you can pass multiple references, in opentelemetry you pass just one. So when translating the many references, the second and further opentracing references could become opentelemetry links. I don't see a way to set it up.
  • Span kind setup - no way to set it up.
  • How to support FinishWithOptions in opentracing Tracer?
  • Changing the name of the span after creating it (opentracing has the API for setting the operation name, which I translate to span name)

jmacd
jmacd reviewed Aug 15, 2019
View reviewed changes
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from 170c7ab to 4d430de Compare August 21, 2019 15:51
@krnowak krnowak mentioned this pull request Aug 22, 2019
Merged
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from 4a53c56 to 8877950 Compare August 29, 2019 13:09
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from 254c30c to 735e554 Compare September 5, 2019 15:30
jmacd
jmacd approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Looks great. I'd like a high-level comment explaining all the Context machinery here.

experimental/bridge/opentracing/internal/doc.go Show resolved Hide resolved
experimental/bridge/opentracing/internal/mock.go Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/lib.go Outdated Show resolved Hide resolved
experimental/bridge/opentracing/wrapper.go Show resolved Hide resolved
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from b499dbd to 6ea7100 Compare September 18, 2019 16:07
@krnowak
Copy link
Member Author

krnowak commented Sep 18, 2019

Updated, ready for review, so the "do not review" label can be dropped.

jmacd
jmacd approved these changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

💯

@jmacd jmacd mentioned this pull request Sep 19, 2019
Closed
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from 6ea7100 to cb8cb0b Compare September 19, 2019 18:09
@krnowak
Copy link
Member Author

krnowak commented Sep 19, 2019

Fixed conflicts.

@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from cb8cb0b to 69f2688 Compare September 19, 2019 19:17
@krnowak
Copy link
Member Author

krnowak commented Sep 19, 2019

Rebased again to include the deletion of the registry package, so the bridge code needed updates. Squashed the commits.

@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from 69f2688 to 707ea58 Compare September 21, 2019 18:57
@krnowak
Copy link
Member Author

krnowak commented Sep 21, 2019

Updated and rebased on top of the current master, so it builds (span interface got link stuff, so test were failing to build).

jmacd
jmacd approved these changes Sep 23, 2019
View reviewed changes
experimental/bridge/opentracing/doc.go
// LogFields() function, so when the call to the function gets
// translated to OpenTelemetry AddEvent() function, an empty context
// is passed.
package opentracing // import "go.opentelemetry.io/experimental/bridge/opentracing"
Copy link
Contributor

Choose a reason for hiding this comment

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

💥 🏅 💥 🏅 💥

experimental/bridge/opentracing/wrapper.go Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Sep 24, 2019 •
edited
Loading

This is ready to merge, once the build is fixed again. Sorry, I may be responsible, but since this is a fork I can't edit the branch myself (I think).

@iredelmeier
Copy link
Member

Looks like the problem is from this commit, which is what replaced the Injector with the Propagator.

Also, super small thing: thoughts on rebasing to get rid of the merge commit?

@rghetia
Copy link
Contributor

rghetia commented Sep 24, 2019

Looks like the problem is from this commit, which is what replaced the Injector with the Propagator.

Also, super small thing: thoughts on rebasing to get rid of the merge commit?

I have resolved the conflict but I cannot update this PR. There isn't much we can do other than open a new PR and give credits to @krnowak in the comments.

@jmacd
Copy link
Contributor

jmacd commented Sep 25, 2019

Let's just let @krnowak repair it.
(I'm anti-rebase, for what it's worth.)

@krnowak
Allow specifying custom timestamps for events
33ef85c 
Adding event with timestamp is not yet a part of the OpenTelemetry
specification, but this function will come in handy when implementing
the OpenTracing bridge.
@krnowak
Add opentracing bridge, wrapper tracer and migration interfaces
5f06587 
There are some features missing - setting up links and span kind;
context propagation will only work between two OpenTracing bridges.
@krnowak
Add some tests for the opentracing bridge
60c5d1f 
The tests mostly check various aspects of the cooperation between
OpenTracing and OpenTelemetry APIs.
@krnowak krnowak force-pushed the krnowak/opentracing-bridge branch from dae9c88 to 60c5d1f Compare September 25, 2019 06:06
@krnowak
Copy link
Member Author

krnowak commented Sep 25, 2019

Fixed and updated.

@rghetia rghetia merged commit 339ca2d into open-telemetry:master Sep 25, 2019
@jmacd jmacd mentioned this pull request Sep 26, 2019
Closed
@krnowak krnowak deleted the krnowak/opentracing-bridge branch September 28, 2019 11:20
@krnowak krnowak mentioned this pull request Oct 9, 2019
Closed
6 tasks
hstan referenced this pull request in hstan/opentelemetry-go Oct 15, 2020
@dependabot @MrAlias
Bump google.golang.org/grpc in /instrumentation/gin-gonic/gin (#98)
2638e7b 
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.29.1 to 1.30.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.29.1...v1.30.0)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tyler Yahn <[email protected]>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rghetia rghetia rghetia approved these changes

@jmacd jmacd jmacd approved these changes

@tedsuo tedsuo tedsuo approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@pjanotti pjanotti Awaiting requested review from pjanotti

@sjkaris sjkaris Awaiting requested review from sjkaris

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

Successfully merging this pull request may close these issues.
6 participants
@krnowak @jmacd @iredelmeier @rghetia @tedsuo @pellared