Update to jaeger-lib 2 and latest sha for jaeger-client-go, to pick u…

[objectiser]

…p refactored metric names

Signed-off-by: Gary Brown [email protected]

Which problem is this PR solving?

Updating to latest jaeger-lib (version 2) and jaeger-client-go (sha currently as not yet released). This is to pick up changes to the metric names.

Short description of the changes

Updated versions - as the metrics factory approach changed in jaeger-lib 2 this required updates throughout the code.

NOTE: Will need to change the namespace used with the go client, to use the base metrics factory, to avoid jaeger_client_jaeger_tracer.... prefix and instead just have jaeger_tracer..... Should this be added to this PR or be done separately?

[codecov]

Codecov Report

Merging #1282 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1282   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         161     161           
  Lines        7185    7193    +8     
======================================
+ Hits         7185    7193    +8

Impacted Files	Coverage Δ
cmd/ingester/app/consumer/offset/manager.go	100% <100%> (ø)	⬆️
cmd/agent/app/testutils/fixture.go	100% <100%> (ø)	⬆️
storage/spanstore/metrics/write_metrics.go	100% <100%> (ø)	⬆️
cmd/ingester/app/consumer/deadlock_detector.go	100% <100%> (ø)	⬆️
cmd/ingester/app/processor/decorator/retry.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/tchannel/collector_proxy.go	100% <100%> (ø)	⬆️
plugin/storage/cassandra/spanstore/reader.go	100% <100%> (ø)	⬆️
storage/spanstore/metrics/decorator.go	100% <100%> (ø)	⬆️
plugin/storage/cassandra/spanstore/writer.go	100% <100%> (ø)	⬆️
cmd/collector/app/builder/span_handler_builder.go	100% <100%> (ø)	⬆️
... and 11 more

---

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 d646a3c...5e2da34. Read the comment docs.

[pavolloffay]

Just one question related to jaeger client, LGTM otherwise

[pavolloffay]

has the client been updated to use lib v2? If no is it a problem?

[objectiser]

Yes, https://github.com/jaegertracing/jaeger-client-go/blob/master/Gopkg.toml#L23

[pavolloffay]

Then could we pin this to a version rather than a commit?

[objectiser]

This was suggested by @yurishkuro jaegertracing/jaeger-client-go#346 (comment)

[pavolloffay]

thanks for the pointer. It's all good

[pavolloffay]

Does this PR introduce any breaking changes? Even in produced matrics?

[objectiser]

@pavolloffay Yes metric names will be changed - so a note will need to be added to the change log.

I could do it with this PR - but one thing that needs to be decided is whether to also make the jaeger_client_jaeger_tracer -> jaeger_tracer namespace change in this PR or a separate PR?

[pavolloffay]

I could do it with this PR - but one thing that needs to be decided is whether to also make the jaeger_client_jaeger_tracer -> jaeger_tracer namespace change in this PR or a separate PR?

As you prefer. Could you please also add a changelog (separater PR is fine)?

[objectiser]

@pavolloffay Just checked the expvar output, and the jaeger_tracer metrics are inconsistent, as they use a _ - whereas all other components use jaeger.<component> - so think it would be better if I update the jaeger go client first and then update the sha in this PR. Is that ok with you?

[objectiser]

@pavolloffay Have created jaegertracing/jaeger-client-go#364 to fix the namespace separator issue - if you think the change is ok, can you merge that PR and I will update the sha in this PR.

[objectiser]

@pavolloffay All the metrics are now consistent - and the changelog has been updated.