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

Add open telemetry sample #320

Closed
wants to merge 1 commit into from

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Add open telemetry sample

Leaving as draft for two reasons:

  • Due to some samples depending on an old server version I couldn't add newer otel SDKs so I temporarily removed them
  • The unit test is not getting the correct spans and I suspect their is a bug with interceptors in the test enviorment

@emanuelef
Copy link
Contributor

Hi, I have a similar problem with my PR: #296
That PR has been there for months but cannot be merged because of #293

@Quinn-With-Two-Ns
Copy link
Contributor Author

@emanuelef Sorry I completely missed your PR or that it was blocked. I will create a PR to upgrade the server dependency.

@emanuelef
Copy link
Contributor

@Quinn-With-Two-Ns I see as part of this PR some samples have been removed, did you need that to be able to use OpenTelemetry modules ? I might need to do the same in my PR, provided it makes sense to have both PRs been merged.

@Quinn-With-Two-Ns
Copy link
Contributor Author

@emanuelef No samples should be removed, I just did that to unblock myself from dependency issues. It doesn't make sense to merge both out PRs. I am fine merging yours and I may make a follow up PR to add a few more cases to it to align it with our other SDKs otel examples

@emanuelef
Copy link
Contributor

The imports where I'm having problems as well are:

In grpc-proxy and codec-server:
go.temporal.io/server/common/authorization
go.temporal.io/server/common/config

What would be the new way to import those modules ?

@emanuelef
Copy link
Contributor

@Quinn-With-Two-Ns The PR #296 is ready to be merged

@Quinn-With-Two-Ns
Copy link
Contributor Author

@emanuelef Thanks, sorry was out of the office.

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