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

Initial implementation of the OpenTracing bridge. #197

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

pcwiese
Copy link
Contributor

@pcwiese pcwiese commented Aug 29, 2019

This is, where possible, a direct port of the current Java shim. There are some holes/TODOS. Notably:

  • Missing the Binary injection/extraction of the span context. This doesn't exist with the current implementation of OpenTracing yet.
  • Missing explicit timestamps for calls to OpenTracing ISpan::Log.
  • Missing explicit timestamps for calls to OpenTracing ISpan::Finish. Requires implementation of https://github.com/open-telemetry/oteps/blob/master/text/0002-remove-spandata.md
  • Various calls to set/get baggage are not implemented. Requires OpenTelemetry DistributedContext to be finalized.

Other oddities worth nothing:

  • The SpanBuilderShim has some conditional code for calling OpenTel SetCreateChild(true|false)
  • The ScopeManagerShim is actively tracking scopes returned by calls to OpenTel WithSpan and associating those with the OpenTel ISpan used when calling WithSpan.

I tested this shim against an ASP.NET Core 3 Preview 8 service wired up with OpenTracing netcore and grpc collectors and exporting to Jaeger backend.

The included tests are unit tests only; no E2E tests.

This is, where possible, a direct port of the current Java shim. There are some holes/TODOS. Notably:

-Missing the Binary injection/extraction of the span context. This doesn't exist with the current implementation of OpenTracing yet.
-Missing explicit timestamps for calls to OpenTracing ISpan::Log.
-Missing explicit timestamps for calls to OpenTracing ISpan::Finish
-Various calls to set/get baggage are not implemented. Requires OpenTelemetry DistributedContext to be finalized.

Other oddities worth nothing:

-The SpanBuilderShim has some conditional code for calling OpenTel SetCreateChild(true|false)
-The ScopeManagerShim is actively tracking scopes returned by calls to OpenTel WithSpan and associated those with the OpenTel ISpan used when calling WithSpan.

I tested this shim against an ASP.NET Core 3 Preview 8 service wired up with OpenTracing netcore and grpc collectors and exporting to Jaeger backend.

The included tests are unit tests only; no E2E tests.
@pcwiese pcwiese force-pushed the pwiese/opentracing-bridge branch from b90770b to 4ce8cc0 Compare August 29, 2019 21:29
Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -54,7 +54,7 @@ public SpanContext Extract<T>(T carrier, Func<T, string, IEnumerable<string>> ge
return SpanContext.Blank;
}

var traceparent = traceparentCollection.First();
var traceparent = traceparentCollection?.First();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this change is in this PR?

Copy link

Choose a reason for hiding this comment

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

i think it's an accidental bugfix

@lmolkova
Copy link

lmolkova commented Sep 6, 2019

it seems @pcwiese is out on the leave. I'll merge it before it diverges from master

@lmolkova lmolkova merged commit 541390f into open-telemetry:master Sep 6, 2019
sywhang pushed a commit to sywhang/opentelemetry-dotnet that referenced this pull request Sep 6, 2019
This is, where possible, a direct port of the current Java shim. There are some holes/TODOS. Notably:

-Missing the Binary injection/extraction of the span context. This doesn't exist with the current implementation of OpenTracing yet.
-Missing explicit timestamps for calls to OpenTracing ISpan::Log.
-Missing explicit timestamps for calls to OpenTracing ISpan::Finish
-Various calls to set/get baggage are not implemented. Requires OpenTelemetry DistributedContext to be finalized.

Other oddities worth nothing:

-The SpanBuilderShim has some conditional code for calling OpenTel SetCreateChild(true|false)
-The ScopeManagerShim is actively tracking scopes returned by calls to OpenTel WithSpan and associated those with the OpenTel ISpan used when calling WithSpan.

I tested this shim against an ASP.NET Core 3 Preview 8 service wired up with OpenTracing netcore and grpc collectors and exporting to Jaeger backend.

The included tests are unit tests only; no E2E tests.
@pcwiese
Copy link
Contributor Author

pcwiese commented Sep 6, 2019 via email

@austinlparker
Copy link
Member

Thanks @lmolkova, and congratulations @pcwiese!

@lmolkova
Copy link

lmolkova commented Sep 6, 2019

@pcwiese congratulations!

lmolkova pushed a commit that referenced this pull request Sep 6, 2019
* Remove ExportComponent from README

* Initial implementation of the OpenTracing bridge. (#197)

This is, where possible, a direct port of the current Java shim. There are some holes/TODOS. Notably:

-Missing the Binary injection/extraction of the span context. This doesn't exist with the current implementation of OpenTracing yet.
-Missing explicit timestamps for calls to OpenTracing ISpan::Log.
-Missing explicit timestamps for calls to OpenTracing ISpan::Finish
-Various calls to set/get baggage are not implemented. Requires OpenTelemetry DistributedContext to be finalized.

Other oddities worth nothing:

-The SpanBuilderShim has some conditional code for calling OpenTel SetCreateChild(true|false)
-The ScopeManagerShim is actively tracking scopes returned by calls to OpenTel WithSpan and associated those with the OpenTel ISpan used when calling WithSpan.

I tested this shim against an ASP.NET Core 3 Preview 8 service wired up with OpenTracing netcore and grpc collectors and exporting to Jaeger backend.

The included tests are unit tests only; no E2E tests.
@pcwiese pcwiese deleted the pwiese/opentracing-bridge branch October 2, 2019 21:52
Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022
…rces due to exception "The SSL connection could not be established" (open-telemetry#208)

* open-telemetry#197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established"

Issue Description:
With the current code, it requires an additional step of installing kubernetes self-signed certificate to the trusted store as .NET core expects them to the trsuted store to make a secure connection.

Fix Provided:
ServerCertificationValidationProvider - Safely loads the certificate to the trusted collection along with ServerSideValidation. This would avoid the unnecessary step of installing the certificate manually.
Handler - Creates HttpClientHandler with client certificate.

(cherry picked from commit 41e68d3c55ec3c61604a9c5ba07242747f5a17e9)

* PR Review Comments

(cherry picked from commit 0b5d9a9360c35e5db93f92e22fbea5f43b1adfd0)

* Address PR comments

(cherry picked from commit 4aa1c6213aabc78e60b841632ebc4db03cd4986c)

* Remove unused import

* Added Unit Tests

* Removed unused directive

* Added Trait for codecov build

* Use temporary files to store test certificate

* Fixed File Access Issue in Unit Test

* Use TempFileName for Unit Tests

* Added Debug logging for build test
Fixed For loop tries

* Revert "Added Debug logging for build test"

This reverts commit b9823252f545324efc0031b0b654462085cb1f68.

* Fixed tries logic in for loop

Co-authored-by: Prashant Srivastava <[email protected]>
Co-authored-by: Igor Kiselev <[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.

3 participants