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

Bump Jaeger version to 1.14.0 (latest) #349

Merged
merged 8 commits into from
Sep 23, 2019

Conversation

annanay25
Copy link
Contributor

Signed-off-by: Annanay [email protected]

Objective

Changes

  • Refactor healthcheck and jaegerreceiver modules

cc @yurishkuro @jpkrohling

@annanay25
Copy link
Contributor Author

I signed it

Signed-off-by: Annanay <[email protected]>
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

goimports FAILED => fix the following goimports errors:
diff -u extension/healthcheckextension/healthcheckextension.go.orig extension/healthcheckextension/healthcheckextension.go
--- extension/healthcheckextension/healthcheckextension.go.orig	2019-09-14 20:34:39.697053255 +0000
+++ extension/healthcheckextension/healthcheckextension.go	2019-09-14 20:34:39.697053255 +0000
@@ -20,8 +20,9 @@
 	"strconv"
 
 	"github.com/jaegertracing/jaeger/pkg/healthcheck"
-	"github.com/open-telemetry/opentelemetry-service/extension"
 	"go.uber.org/zap"
+
+	"github.com/open-telemetry/opentelemetry-service/extension"
 )

Please fix.

Signed-off-by: Annanay <[email protected]>
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

@annanay25 there are test failures because of the health extension changes:

--- FAIL: TestHealthCheckExtensionUsage (0.01s)
    healthcheckextension_test.go:60: 
        	Error Trace:	healthcheckextension_test.go:60
        	Error:      	Not equal: 
        	            	expected: 204
        	            	actual  : 200
        	Test:       	TestHealthCheckExtensionUsage
--- FAIL: TestHealthCheckExtensionPortAlreadyInUse (0.00s)
    healthcheckextension_test.go:94: 
        	Error Trace:	healthcheckextension_test.go:94
        	Error:      	Received unexpected error:
        	            	listen tcp :35499: bind: address already in use
        	Test:       	TestHealthCheckExtensionPortAlreadyInUse
--- FAIL: TestHealthCheckMultipleStarts (0.00s)
    healthcheckextension_test.go:116: 
        	Error Trace:	healthcheckextension_test.go:116
        	Error:      	Received unexpected error:
        	            	listen tcp :35885: bind: address already in use
        	Test:       	TestHealthCheckMultipleStarts
FAIL
coverage: 87.5% of statements
FAIL	github.com/open-telemetry/opentelemetry-service/extension/healthcheckextension	0.031s

and

--- FAIL: TestApplication_StartUnified (0.04s)
    service_test.go:63: app didn't reach ready state
FAIL
coverage: 69.9% of statements
FAIL	github.com/open-telemetry/opentelemetry-service/service	0.134s

@annanay25
Copy link
Contributor Author

@pjanotti - I've fixed all the healthcheck tests, not sure why TestApplication_StartUnified is failing - just checking if I'm missing something obvious. I'll continue debugging tomorrow.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

I think you need to update https://github.com/open-telemetry/opentelemetry-service/pull/349/files#diff-469ef5f4321fa9af5e645d98ab1c5348R78:

return resp.StatusCode == http.StatusNoContent
->
return resp.StatusCode == http.StatusOK

Signed-off-by: Annanay <[email protected]>
@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #349 into master will decrease coverage by <.01%.
The diff coverage is 87.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
- Coverage    74.4%   74.39%   -0.01%     
==========================================
  Files         115      115              
  Lines        6708     6725      +17     
==========================================
+ Hits         4991     5003      +12     
- Misses       1470     1474       +4     
- Partials      247      248       +1
Impacted Files Coverage Δ
receiver/jaegerreceiver/trace_receiver.go 76.92% <83.33%> (-0.47%) ⬇️
...nsion/healthcheckextension/healthcheckextension.go 92.85% <92.3%> (-7.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63362d5...bb679ca. Read the comment docs.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making this change.

@tigrannajaryan
Copy link
Member

Reviews, please have a look.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Hi @annanay25 - please, extract the common code between the two SubmitBatches implementation as suggested by @owais.

receiver/jaegerreceiver/trace_receiver.go Outdated Show resolved Hide resolved
@annanay25
Copy link
Contributor Author

Thanks for the reviews @pjanotti, @owais - I've addressed the comments.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @annanay25

@pjanotti pjanotti merged commit 2393774 into open-telemetry:master Sep 23, 2019
@annanay25
Copy link
Contributor Author

Thanks!

@annanay25 annanay25 deleted the bump-jaeger-version branch November 26, 2019 21:36
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
…y#349)

* Remove AddLink & Link from Span Interface

I have remove AddLink and Link from the interface and all it refereneces and replaced AddLink with addlink, Also Removed respective unit tests

Signed-off-by: vineeth <[email protected]>

* removing the unused code from unit tests

Signed-off-by: VineethReddy02 <[email protected]>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
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.

Upgrade Jaeger to latest version
6 participants