Skip to content

Conversation

@sivchari
Copy link
Contributor

Description

Use grpc.NewClient instead of grpc.DialContext

Link to tracking issue

Fixes #13632

Testing

Documentation

@sivchari sivchari requested a review from a team as a code owner August 19, 2025 02:35
@sivchari sivchari requested a review from codeboten August 19, 2025 02:35
@sivchari sivchari changed the title move from DialContext to NewClient [chore] move from DialContext to NewClient Aug 19, 2025
@codecov
Copy link

codecov bot commented Aug 19, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.09%. Comparing base (82c3e02) to head (9c842ac).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
config/configgrpc/configgrpc.go 60.00% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (60.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13663      +/-   ##
==========================================
- Coverage   92.10%   92.09%   -0.02%     
==========================================
  Files         668      668              
  Lines       41377    41381       +4     
==========================================
- Hits        38112    38110       -2     
- Misses       2227     2231       +4     
- Partials     1038     1040       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

@evan-bradley
Copy link
Contributor

evan-bradley commented Aug 19, 2025

I suspect the contrib test failures are due to the fact that NewClient doesn't initiate a connection unlike DialContext, we should be able to fix that somewhat easily.

@sivchari Can you confirm that this doesn't reintroduce #11537? If possible, a basic test would be nice.

@sivchari
Copy link
Contributor Author

sivchari commented Sep 2, 2025

Sorry for late, I added the condition to check whether the connection is established correctly

@dmathieu
Copy link
Member

dmathieu commented Sep 2, 2025

Were you able to explicitly confirm that the issue in #11537 isn't reintroduced here?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 17, 2025
@github-actions github-actions bot removed the Stale label Sep 18, 2025
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 10, 2025
@github-actions github-actions bot removed the Stale label Oct 17, 2025
@dmathieu dmathieu added the ready-to-merge Code review completed; ready to merge by maintainers label Oct 28, 2025
@dmathieu
Copy link
Member

I'm marking this as "ready to merge" based on the number of approvals.
My comment about removing the test for go-grpc behavior is not a blocker.

@dmathieu dmathieu linked an issue Oct 28, 2025 that may be closed by this pull request
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Nov 12, 2025
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 28, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 28, 2025

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

✅ 59 untouched benchmarks
⏩ 20 skipped benchmarks1


Comparing sivchari:pr-13632 (9c842ac) with main (82c3e02)

Open in CodSpeed

Footnotes

  1. 20 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions github-actions bot removed the Stale label Nov 29, 2025
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 16, 2025
@tank-500m
Copy link
Contributor

Hi
is this issue still in progress?

@dmathieu
Copy link
Member

dmathieu commented Jan 5, 2026

For this to be merged, the unit tests need to be fixed.

@gimsesu
Copy link

gimsesu commented Jan 7, 2026

Hi @sivchari,

Thank you for working on this PR. We're looking forward to this change as it would benefit our project that uses the OpenTelemetry Collector.
I saw that the PR has multiple approvals and is marked as "ready-to-merge." As @dmathieu mentioned, it looks like the unit tests need to be fixed before merging.
Is there anything the community can help with to move this forward? Please let us know if there's any way we can assist.
Thanks again for your contribution!

Signed-off-by: sivchari <shibuuuu5@gmail.com>
Signed-off-by: sivchari <shibuuuu5@gmail.com>
Signed-off-by: sivchari <shibuuuu5@gmail.com>
Per review feedback from @dmathieu, this test was testing go-grpc
behavior rather than collector code. The fix in go-grpc has been
confirmed, so this automated test is not necessary.
@sivchari
Copy link
Contributor Author

sivchari commented Jan 7, 2026

Sorry for late response, I missed comments. I removed test, so I think it's ok to be merged.
Thanks.

@dmathieu dmathieu removed the Stale label Jan 7, 2026
@bogdandrutu bogdandrutu added this pull request to the merge queue Jan 8, 2026
Merged via the queue into open-telemetry:main with commit b752ad3 Jan 8, 2026
90 of 95 checks passed
@otelbot
Copy link
Contributor

otelbot bot commented Jan 8, 2026

Thank you for your contribution @sivchari! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

dmitryax added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Jan 12, 2026
Cannot be done in CI due to a new internal module in core. See
https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/20846134948/job/59890092343

Extra changes:
- Add temporary replace statements for
go.opentelemetry.io/collector/internal/componentalias
- Update Coralogix exporter tests after
open-telemetry/opentelemetry-collector#13663
Syedowais312 pushed a commit to Syedowais312/opentelemetry-collector that referenced this pull request Jan 14, 2026
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Use grpc.NewClient instead of grpc.DialContext

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes open-telemetry#13632 

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: sivchari <shibuuuu5@gmail.com>
Syedowais312 pushed a commit to Syedowais312/opentelemetry-collector that referenced this pull request Jan 15, 2026
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Use grpc.NewClient instead of grpc.DialContext

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes open-telemetry#13632 

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: sivchari <shibuuuu5@gmail.com>
@gimsesu gimsesu mentioned this pull request Jan 23, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Code review completed; ready to merge by maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[exporter/otlp] gRPC balancer default behavior [configgrpc] Move to NewClient

10 participants