Skip to content

Conversation

tico88612
Copy link
Contributor

@tico88612 tico88612 commented Apr 7, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

  • make lint
  • make test
  • Try & Error

Checklist

@tico88612 tico88612 requested a review from a team as a code owner April 7, 2024 02:29
@tico88612 tico88612 requested a review from pavolloffay April 7, 2024 02:29
Copy link

codecov bot commented Apr 7, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.18%. Comparing base (7be2e34) to head (41d2e3d).

Files Patch % Lines
cmd/anonymizer/app/query/query.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5336   +/-   ##
=======================================
  Coverage   95.18%   95.18%           
=======================================
  Files         343      343           
  Lines       16779    16780    +1     
=======================================
+ Hits        15971    15972    +1     
+ Misses        609      608    -1     
- Partials      199      200    +1     
Flag Coverage Δ
badger 10.59% <ø> (ø)
cassandra-3.x 18.52% <ø> (ø)
cassandra-4.x 18.52% <ø> (ø)
elasticsearch-5.x 21.00% <ø> (+0.01%) ⬆️
elasticsearch-6.x 21.00% <ø> (+0.01%) ⬆️
elasticsearch-7.x 21.04% <ø> (-0.02%) ⬇️
elasticsearch-8.x 21.24% <ø> (ø)
grpc 14.58% <ø> (-0.02%) ⬇️
kafka 10.21% <ø> (ø)
opensearch-1.x 21.10% <ø> (+0.01%) ⬆️
opensearch-2.x 21.09% <ø> (-0.02%) ⬇️
unittests 91.75% <83.33%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro yurishkuro added the changelog:dependencies Update to dependencies label Apr 8, 2024
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Lgtm, what's with the test failing?

@tico88612
Copy link
Contributor Author

tico88612 commented Apr 8, 2024

Honestly, I have no idea at the moment.

But looking at the CI message, it looks like something is wrong with the connection.
They all end in timeout and bind: address already in use in the gRPC TLS test.

https://github.com/jaegertracing/jaeger/actions/runs/8602860896/job/23573382151?pr=5336#step:7:81
https://github.com/jaegertracing/jaeger/actions/runs/8602860896/job/23573382151?pr=5336#step:7:235

My guess might be Dial and NewClient. The former has passthrough as the default scheme, and the latter has dns. I'll try to see if I can fix testcases.

@yurishkuro
Copy link
Member

don't look at the last error message. I see that these tests timeout (the first error in the logs):

$ go test -test.timeout=10s -v ./cmd/agent/app/reporter/grpc/
..
panic: test timed out after 10s
running tests:
	TestBuilderWithCollectors (10s)
	TestBuilderWithCollectors/with_collector_connection_status_ready (10s)

@tico88612 tico88612 marked this pull request as draft April 9, 2024 12:10
Signed-off-by: tico88612 <17496418+tico88612@users.noreply.github.com>
Signed-off-by: tico88612 <17496418+tico88612@users.noreply.github.com>
Signed-off-by: tico88612 <17496418+tico88612@users.noreply.github.com>
Signed-off-by: tico88612 <17496418+tico88612@users.noreply.github.com>
@tico88612 tico88612 changed the title Upgrade google.golang.org/grpc to 1.63.0 Partial replace deprecated function in grpc-go@v1.64 & Refactor configmanager business logic Apr 9, 2024
@tico88612 tico88612 marked this pull request as ready for review April 9, 2024 16:12
@yurishkuro yurishkuro changed the title Partial replace deprecated function in grpc-go@v1.64 & Refactor configmanager business logic Replace grpc.Dial function that is going to be deprecated in grpc-go@v1.64 Apr 9, 2024
@yurishkuro yurishkuro merged commit aa9cefc into jaegertracing:main Apr 9, 2024
@yurishkuro yurishkuro mentioned this pull request Apr 26, 2024
4 tasks
varshith257 pushed a commit to varshith257/jaeger that referenced this pull request May 3, 2024
…v1.64 (jaegertracing#5336)

## Which problem is this PR solving?
- Related jaegertracing#5330 

## Description of the changes
- Partial replace deprecated function in `grpc-go@v1.64`
  - `grpc.Dial` => `grpc.NewClient`
  - `grpc.DialContext` => `grpc.NewClient`
- FYI:
jaegertracing#5330 (comment)
- Refactor configmanager GetSamplingStrategy business logic
  - Fixed related test

## How was this change tested?
- make lint
- make test
- Try & Error

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: tico88612 <17496418+tico88612@users.noreply.github.com>
Signed-off-by: Vamshi Maskuri <gwcchintu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:dependencies Update to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants