search

jaegertracing / jaeger-client-go

🛑 This library is DEPRECATED!
https://jaegertracing.io/
Apache License 2.0
1.39k stars 288 forks source link

use go modules #595

Closed Nerzal closed 2 years ago

Nerzal commented 3 years ago

Which problem is this PR solving?

Resolves #382

Short description of the changes

Started new clean PR (closed #583)

Make file passes locally

Tests pass locally: grafik

Nerzal commented 3 years ago

Looks like my VSCode auto applied gofmt -w -s Is that an issue? Lots of code have been reformated.

But when running make fmt nothing changes anymore :thinking:

yurishkuro commented 3 years ago

We need to go to new major version because we already have lots of version tags that apply to the old namespace.

Nerzal commented 3 years ago

We need to go to new major version because we already have lots of version tags that apply to the old namespace.

So you mean something like v4? We should currently be on v2, so i thought v3 would fit :D

yurishkuro commented 3 years ago

V3 is fine since the current code maxes or at 2.x

Nerzal commented 3 years ago

Would it be possible to approve the usage of the Github pipelines, so i can fix possible errors inside the pipelines? :)

yurishkuro commented 3 years ago

Approved runs, but all CI can also be run locally.

codecov[bot] commented 3 years ago

Codecov Report

Merging #595 (30c85cc) into dev-v3 (7aa7af5) will increase coverage by 0.06%. The diff coverage is n/a.

:exclamation: Current head 30c85cc differs from pull request most recent head 5e31c4b. Consider uploading reports for the commit 5e31c4b to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           dev-v3     #595      +/-   ##
==========================================
+ Coverage   88.58%   88.64%   +0.06%     
==========================================
  Files          61       61              
  Lines        3328     3328              
==========================================
+ Hits         2948     2950       +2     
+ Misses        252      251       -1     
+ Partials      128      127       -1
Impacted Files Coverage Δ
baggage_setter.go 100.00% <ø> (ø)
config/config.go 95.33% <ø> (ø)
config/config_env.go 96.77% <ø> (ø)
config/options.go 100.00% <ø> (ø)
crossdock/client/client.go 72.72% <ø> (ø)
crossdock/client/trace.go 66.66% <ø> (ø)
crossdock/endtoend/handler.go 85.36% <ø> (ø)
crossdock/server/server.go 64.70% <ø> (ø)
crossdock/server/trace.go 80.48% <ø> (ø)
internal/baggage/remote/options.go 71.42% <ø> (ø)
... and 28 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 7aa7af5...5e31c4b. Read the comment docs.

Nerzal commented 3 years ago

Well tbh i'm not sure what to do about the 2 CodeQL failures.

yurishkuro commented 3 years ago

I think we can ignore codeql errors, it seems to rain before the or as well, need to look at it separately

yurishkuro commented 3 years ago

@Nerzal we just discussed this change on the Jaeger call and decided that it would be a good idea to combine the major version / namespace change with the clean-up items that have accumulated in the code base and require breaking changes.

I've created a new branch dev-v3, could you please repoint the PR to that branch? We'll do the clean-ups there and then merge the whole thing to master.

$ grep -rn 'TODO (breaking change)' .
./sampler_v2.go:55:// TODO (breaking change) remove this in the next major release
./utils/rate_limiter.go:24:// TODO (breaking change) remove this interface in favor of public struct below
./utils/rate_limiter.go:44:// TODO (breaking change) rename to RateLimiter once the interface is removed
./sampler_remote.go:92:// TODO (breaking change) remove when Sampler V1 is removed
./tracer.go:78:// TODO (breaking change) return *Tracer only, without closer.
./span.go:72:// TODO (breaking change) deprecate in the next major release, use opentracing.Tag instead.
./span.go:79:// TODO (breaking change) deprecate in the next major release, use opentracing.Tag instead.
./sampler.go:46:    // TODO (breaking change) remove this function. See PerOperationSampler.Equals for explanation.
./sampler.go:392:// TODO (breaking change) remove when upgrading everything to SamplerV2
./sampler.go:482:// TODO (breaking change) remove this in the future
yurishkuro commented 3 years ago

cc @joe-elliott

Nerzal commented 3 years ago

Sure, will do. Give me some minutes

Nerzal commented 3 years ago

Heyho, don't wanna stress you out, but is there anything i can do to speed up the process here? :)